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

MNT: add __all__ to top level module #680 #683

Merged
merged 1 commit into from
Jul 15, 2023

Conversation

Kirill888
Copy link
Contributor

@snowman2
Copy link
Member

Thanks @Kirill888 👍, have you verified that this addresses the issue? I am still wondering if this is more of a pylance issue and less of a rioxarray issue.

Side tangent: I am not a fan of __all__ as it adds a list to manually maintain only for the reason to support bad practice.

https://docs.python.org/3/tutorial/modules.html#importing-from-a-package

Although certain modules are designed to export only names that follow certain patterns when you use import *, it is still considered bad practice in production code.

@Kirill888
Copy link
Contributor Author

Side tangent: I am not a fan of all as it adds a list to manually maintain only for the reason to support bad practice.

Sure from X import * should not be encouraged, but adding "harm reduction" measure is not the same as "support".

I see __all__ not as code duplication, but rather as redundancy, an explicit declaration of what constitutes public facing modules/methods offered by this library vs imports that are needed to do something else internally. It's certainly used by things like pylint, jedi. Think of it as part of documentation for the module.

I think maintenance burden of this is minimal, pylint will rightfully complain about imports that are not used internally and also not "exposed" through __all__ mechanism. So it's not just to help users of from rioxarray import *, who would not even notice any change, after this PR.

@snowman2
Copy link
Member

Thanks for your thoughts on __all__ @Kirill888 - seems like my rant has opened a can of worms 😄

I think rioxarray has a small enough list that it is not an issue to add __all__ to the top level __init__.py, so merging this shouldn't be an issue.

Other ranting to feel free to ignore 😄 :

  • pylint and other linters complain about doing from <> import * ref, so I think that is a reasonable solution for damage reduction.
  • The __all__ list in xarray and it is getting quite long - increasing its likelihood to get out of sync from the actual public import list. Not the first time I have encountered this.
  • __all__ it is not presently used by libraries like requests
  • "Package authors may also decide not to support it, if they don’t see a use for importing * from their package." ref
  • Alternate solutions that may be easier to maintain: "single_leading_underscore: weak “internal use” indicator. E.g. from M import * does not import objects whose names start with an underscore." ref.

"open_rasterio",
"set_options",
"show_versions",
"__author__",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure __author__ and __version__ are useful to add to __all__? I was thinking only functions without an _ in the prefix should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, if one thinks of it as "a list of exported symbols", then yes. Pylint and numpy DO include __version__ in __all__.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to stare at worms sometimes 👁️ 🪱 🪱 😄 .

I agree it's a bit annoying to have that separation between implementation and "extern" marker. I guess this is one of the trade-offs of using highly dynamic multi-paradigm language - community needs to develop and apply conventions that are not enforced by a strict spec and type system. And those conventions evolve over time, __all__ might have started as a mechanism for controlling star imports, but it is now used by various code analysis tools as well, even if it's not official enough to be reflected in official language docs and not adopted by all of the popular libraries. I also won't be surprised if mypy offers something in that space as well, that maybe, requests is using instead?

There are probably tools already out there that help with the issue of keeping __all__ in sync with the code. I never looked, as it's not a pain-point for me. I do forget to define __all__ in public sub-modules that have method implementations in them, and usually linting tools are lenient and use leading _ as a heuristic for deciding on public interface, so no one notices any issues. I also forget to add new public methods to sphinx docs, but I still think that explicit decision on what should and should not be documented, should or should not be considered part of the public interface is a reasonable approach. Sure, it costs a little bit extra at "create/maintain" stage, but also benefits "use" stage enough to bother.

I usually do not forget to add __all__ in __init__.py files that have no implementation code because of unused-import warning pylint generates in this case. I have been slowly moving towards having private sub-modules with explicit public interface sub-modules that just import and re-export methods from internal implementations, as this allows for greater flexibility in refactoring.

@snowman2
Copy link
Member

Thanks again @Kirill888 👍

@snowman2 snowman2 merged commit 5234747 into corteva:master Jul 15, 2023
@snowman2 snowman2 added this to the 0.14.1 milestone Jul 15, 2023
@snowman2 snowman2 modified the milestones: 0.14.1, 0.15.0 Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"'open_rasterio' is not exported from module 'rioxarray'" error reported by Pylance linting
2 participants