refactor: Avoid implicit reexport in submodules

[l0b0]

Related Issue(s): #332

Description: I couldn't do the equivalent changes anywhere in the serialization submodule without running into circular imports, so I've postponed that until it's easier to fix.

PR Checklist:

• Code is formatted (run pre-commit run --all-files)
• Tests pass (run scripts/test)
• Documentation has been updated to reflect changes, if applicable
• This PR maintains or improves overall codebase code coverage.
• Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

[codecov-commenter]

Codecov Report

Merging #539 (6ef6656) into main (0ba1a1d) will increase coverage by 0.03%.
The diff coverage is 93.63%.

@@            Coverage Diff             @@
##             main     #539      +/-   ##
==========================================
+ Coverage   94.46%   94.49%   +0.03%     
==========================================
  Files          71       71              
  Lines       10728    10795      +67     
  Branches     1075     1075              
==========================================
+ Hits        10134    10201      +67     
  Misses        419      419              
  Partials      175      175              

Impacted Files	Coverage Δ
pystac/extensions/pointcloud.py	95.50% <79.16%> (+0.06%)	⬆️
pystac/extensions/item_assets.py	67.85% <81.81%> (+1.19%)	⬆️
pystac/extensions/hooks.py	81.35% <83.33%> (+0.65%)	⬆️
pystac/extensions/datacube.py	62.73% <85.71%> (+0.57%)	⬆️
pystac/extensions/label.py	93.78% <86.36%> (+0.12%)	⬆️
pystac/extensions/sar.py	98.60% <89.47%> (+0.01%)	⬆️
pystac/extensions/version.py	96.26% <94.73%> (+0.18%)	⬆️
pystac/extensions/base.py	94.02% <100.00%> (+0.58%)	⬆️
pystac/extensions/eo.py	93.24% <100.00%> (+0.12%)	⬆️
pystac/extensions/file.py	92.30% <100.00%> (+0.24%)	⬆️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ba1a1d...6ef6656. Read the comment docs.

[duckontheweb]

Sorry, I just realized I merged some changes from a more recent PR that created conflicts. I'm happy to rebase and resolve these if you want.

[duckontheweb]

I'm +1 for this change, but I'd like to be sure others are on board as well.

[schwehr]

Some groups have style guides and other reasons to follow specific patterns. The one I'm suggesting is documented here: https://google.github.io/styleguide/pyguide.html#22-imports

[schwehr]

I prefer module only imports. e.g. These are a good balance between import pystac which causes trouble and not having to worry about getting more symbols out of any particular module. So as long as modules names aren't mutated, then the imports do not have to change.

from pystac import asset
from pystac import summaries
from pystac import stac_object
from pystac import collection
from pystac import errors
from pystac import summaries

This also means it's a lot easier to find anything in use from a module within the current file even if the names are not as simple as they often are. e.g. a simple search for asset. will find asset.Asset and asset.SomeHelper

[l0b0]

That's a nice compromise. My main question would be whether it helps with avoiding circular imports, which are currently plaguing the code base. For example, running isort will immediately break the entire code base, and there seems to be a bunch of duplicate imports (import pystac followed by from pystac import foo) which, if merged, also cause circular import errors.

[schwehr]

from above, this would be summaries: summaries.Summaries. That's a bit wordy, but really obvious where it comes from.

[l0b0]

Yeah, fixing the comments is another can of worms I'm not comfortable with tackling until we've nailed the imports themselves. I'm guessing they should resolve within the current set of imports, that is, if I from pystac import summaries the comment should be summaries: summaries.Summaries as you said, rather than summaries: pystac.summaries.Summaries.

[schwehr]

Here I would suggest: from pystac.extentions import base. Then it's base.PropertiesExtension in the code.

[duckontheweb]

The one I'm suggesting is documented here: https://google.github.io/styleguide/pyguide.html#22-imports

I like this approach for the reasons that @schwehr mentions in this comment. It also keeps our import statements a bit shorter and means that classes/functions with the same name in different modules wouldn't conflict. We would have to resolve some shadowing issues since we use variable names like link in the code and these would shadow imports like from pystac import link.

If we were to adopt this, I would also propose that in cases where we are importing a class purely for type annotations we always put it in a if TYPE_CHECKING: block and import the class directly for consistency with the way typing and typing_extensions types are imported.

[l0b0]

Sorry, I just realized I merged some changes from a more recent PR that created conflicts. I'm happy to rebase and resolve these if you want.

Thanks, that would be helpful. Then I can start changing all these PRs to use the import strategy suggested by @schwehr.

[l0b0]

I'm going to redo these PRs from scratch, so this may take a while…

[duckontheweb]

Sorry, I just realized I merged some changes from a more recent PR that created conflicts. I'm happy to rebase and resolve these if you want.

Thanks, that would be helpful. Then I can start changing all these PRs to use the import strategy suggested by @schwehr.

@l0b0 Not sure if this is still needed if you are starting from scratch, but I rebased this onto main.

[lossyrob]

I'm -0 on this change. I really like the simplicity of just using import pystac inside the library and going from there. I'm confused about what we buy by making the import list more verbose.

That said, I don't feel that strongly and if people think it will really improve the maintainability in the long run then I'm ok with these sort of changes for the sake of strictness.

I do feel strongly that, as a user, I don't want to have to go digging for imports to use the types of PySTAC. Having a single import pystac and being able to work from there without having to worry about more type or submodule imports is a good developer experience, and I'd like to maintain that ability (which may be more of a response to #543 (comment)). This will be especially true of users who aren't using the same tooling as someone with a more full development environment - think about the data scientists using notebooks that don't have the same autoimport functionality we might be working with as library developers, but who are and will be (I believe) an increasing percentage of the user base. This is already something I've received some negative feedback on with the removal of the magic .ext method and requiring extensions be verbosely imported instead, which is a change I made for more type-aware coding (and I'll stand by that, as the .ext method would never be able to even give tab completion, errored in strange ways and was hard to debug).

I'm not particularly in favor of the "only import submodules rule", but that's just not the way I've coded Python. Again, if that's what folks agree is a good way to go for the internal library development, then I'm willing to go with it - it feels like preference tuning at this point which I guess marks a good point in the library's lifecycle if we're getting to that fine of detail. I'd just keep the developer experience of downstream users who don't have strongly typed tooling in mind when making changes that will affect the import strategy for those users (which may not apply to this PR but is more a general comment).