Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make zarr.core private #2621

Closed
jhamman opened this issue Jan 2, 2025 · 11 comments · Fixed by #2669
Closed

Make zarr.core private #2621

jhamman opened this issue Jan 2, 2025 · 11 comments · Fixed by #2669
Milestone

Comments

@jhamman
Copy link
Member

jhamman commented Jan 2, 2025

We seem to be converging on using a leading underscore to indicate private modules. Should we do this for zarr.core or can we simply document the intent of the module?

@dstansby
Copy link
Contributor

dstansby commented Jan 3, 2025

Is it intended to be private? If so, I'd definitely add a leading underscore to mark it as private.

@jhamman
Copy link
Member Author

jhamman commented Jan 3, 2025

Yes, the intent is to make it private.

@dstansby
Copy link
Contributor

dstansby commented Jan 3, 2025

I had a quick go, and there seem to be bits that need to be public (at least zarr.core.config, zarr.core.buffer.Buffer). Making the whole of core private is going to need someone who knows the codebase well enough (definitely not me 😄) to know what should be public going through carefully and decided what should be public and what should be private.

@jhamman
Copy link
Member Author

jhamman commented Jan 3, 2025

I can take this on.

@dstansby
Copy link
Contributor

dstansby commented Jan 4, 2025

As some small first steps I cleaned up zarr.core.buffer (#2641) and zarr.core.metadata (#2642)

@jhamman
Copy link
Member Author

jhamman commented Jan 6, 2025

@dstansby - what makes you think zarr.core.config needs to be public? The intent, as I see it, is just that zarr.core.config.config is public which is why it is available as zarr.config.

@normanrz - do you think we could move zarr.core.buffer to zarr.buffer? The Buffer ABC seems like it actually belongs in the abc module.

@dstansby
Copy link
Contributor

dstansby commented Jan 6, 2025

what makes you think zarr.core.config needs to be public?

There's user facing documentation in the docstring, but maybe that should now be deleted in favour of the user guide? It looks like BadConfigError isn't used so can be removed, but I think Config should be user facing as it's the type of zarr.config, so maybe that could move to zarr.Config?

@normanrz
Copy link
Member

normanrz commented Jan 6, 2025

@normanrz - do you think we could move zarr.core.buffer to zarr.buffer? The Buffer ABC seems like it actually belongs in the abc module.

Yes, we could do that. And yes, the Buffer and NDBuffer make sense in the abc module.

@jhamman
Copy link
Member Author

jhamman commented Jan 7, 2025

Part 1 done here -> #2664

@jhamman
Copy link
Member Author

jhamman commented Jan 7, 2025

@TomAugspurger
Copy link
Contributor

This came up again in #2876 (review). I'm going to reopen this and look into how we could move everything to zarr._core and deprecate the module with a warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants