corteva / rioxarray

geospatial xarray extension powered by rasterio
https://corteva.github.io/rioxarray
Other
504 stars 80 forks source link

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

Closed Kirill888 closed 12 months ago

Kirill888 commented 12 months ago
snowman2 commented 12 months ago

Thanks @Kirill888 :+1:, 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 commented 12 months ago

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 commented 12 months ago

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

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 :smile: :

snowman2 commented 12 months ago

Thanks again @Kirill888 👍