refactor: Avoid implicit reexport in tests

[l0b0]

Related Issue(s): #332

Description: First step towards strict = True mypy setting.

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.

[duckontheweb]

This looks good to me, but I know there has been some discussion about how to do these imports in other PRs, so I want to make sure I understand the benefits.

It seems like this changes what is implicitly re-exported rather than removing any implicit re-exports (which would be impossible). For instance, we can no longer do this:

from tests.extensions.test_custom import pystac

...but we can now do this:

from tests.extensions.test_custom import Link, Asset, RangeSummary, ...

Is the ultimate goal here to help eliminate circular imports or is there another aim? Also, if there are some documented best practices on this that I've missed feel free to just link to those and I can read up on it myself; this is mostly for my own understanding.

Thanks!

[duckontheweb]

I guess maybe my real question is why mypy cares about this in strict mode...

[l0b0]

This looks good to me, but I know there has been some discussion about how to do these imports in other PRs, so I want to make sure I understand the benefits.

It seems like this changes what is implicitly re-exported rather than removing any implicit re-exports (which would be impossible). For instance, we can no longer do this:

from tests.extensions.test_custom import pystac

...but we can now do this:

from tests.extensions.test_custom import Link, Asset, RangeSummary, ...

That's right; basically, importing from the place where something is defined rather than indirectly via __init__.py all over the place.

Is the ultimate goal here to help eliminate circular imports or is there another aim? Also, if there are some documented best practices on this that I've missed feel free to just link to those and I can read up on it myself; this is mostly for my own understanding.

When I started the work on this I simply assumed that mypy was doing the right thing, since I'm used to empty __init__.py files in all the projects I've worked on. This assumption was strengthened when I discovered that using direct imports caused a bunch of circular imports to break things. It was further strengthened when I tried running isort on the "main" branch and that also caused circular imports. Not being able to reorder imports seems like we're just on the verge of breaking things at all times, and any new import could break things in unexpected ways.

On the other hand, the documentation mentions how including values in __all__ makes them explicitly reexported. I doubt that using __all__ will allow us to use isort (that is, I don't expect it to prevent circular imports), but I can't be sure without trying. I'll try adding __all__ to some __init__.py file(s) and see if that allows us to use isort and mypy --strict without circular imports.

tl;dr: I might not understand imports as well as I'd hoped, but it looks like emptying out the __init__.py files and splitting things up to avoid any remaining circular imports is the way to go. We'll get rid of a bunch of duplicate imports, there shouldn't be any more ambiguity about where to import from, and it would be easy to stop introducing any further circular imports.

Update: Nope, __all__ = [Collection.__name__] in pystac/__init__.py didn't even quieten mypy --strict down:

$ mypy --strict . | grep 'Module "pystac" does not explicitly export attribute "Collection"'
tests/utils/test_cases.py:7: error: Module "pystac" does not explicitly export attribute "Collection"; implicit reexport disabled  [attr-defined]
tests/test_writing.py:6: error: Module "pystac" does not explicitly export attribute "Collection"; implicit reexport disabled  [attr-defined]
tests/test_collection.py:12: error: Module "pystac" does not explicitly export attribute "Collection"; implicit reexport disabled  [attr-defined]
tests/test_catalog.py:11: error: Module "pystac" does not explicitly export attribute "Collection"; implicit reexport disabled  [attr-defined]
tests/extensions/test_label.py:8: error: Module "pystac" does not explicitly export attribute "Collection"; implicit reexport disabled  [attr-defined]

Update 2: Turns out that was because those files were doing from pystac import Collection in addition to import pystac. I'll try using import pystac everywhere and see if that fixes things.

Update 3: Getting rid of those duplicate imports resulted in reintroducing the circular imports, so that's out.

[duckontheweb]

emptying out the __init__.py files and splitting things up to avoid any remaining circular imports is the way to go.

Yeah, I agree that this is definitely the way to go for both the tests and the code base in general. My only hesitation is that it's nice for users to be able to do something like from pystac import Item rather than from pystac.item import Item so they don't need to remember which module everything is in. I'm guessing that will come up as part of #543, though, so I don't see any reason that we can't merge this PR.

[l0b0]

emptying out the __init__.py files and splitting things up to avoid any remaining circular imports is the way to go.

Yeah, I agree that this is definitely the way to go for both the tests and the code base in general. My only hesitation is that it's nice for users to be able to do something like from pystac import Item rather than from pystac.item import Item so they don't need to remember which module everything is in. I'm guessing that will come up as part of #543, though, so I don't see any reason that we can't merge this PR.

I forgot to mention, I think there is a workaround to get the best of both worlds: avoid reexports inside of pystac itself, but import everything we expect the users to use into the root __init__.py, similar to how it looks now. That way the root __init__.py is only used by pystac users, not within pystac itself, but users still have the same usability as now. I can't think of a reason why this won't work off the top of my head; how about you?

[duckontheweb]

avoid reexports inside of pystac itself, but import everything we expect the users to use into the root __init__.py, similar to how it looks now.

This would definitely be the ideal situation; it would be nice not to change the way users have to import from pystac, if possible. To be honest, I'm not really sure whether it's possible or not (I'm constantly surprised by the Python import system 🙂 ), but my instinct is that resolving the circular imports that come up as part of this PR is going to mean that we're able to do it.

[l0b0]

I've tried to pull apart some of the circular imports, but without success so far. pystac.catalog, pystac.collection and pystac.item all seem to use each other, and that's when excluding the if TYPE_CHECKING imports (which in some cases should be removed since the relevant module class is already in scope). It looks like we'll have to pull some core things apart to remove these circular imports.