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

Structure imports in __init__.py #3365

Closed
wants to merge 1 commit into from
Closed

Conversation

tomchristie
Copy link
Member

Closes #3352

@tomchristie tomchristie added the refactor Issues and PRs related to code refactoring label Oct 25, 2024
@zanieb
Copy link
Contributor

zanieb commented Oct 25, 2024

Ruff supports sorting __all__ blocks — it's a bit different than this (and I'm not sure what happens if you create categories with comments), but it might be nice to automate.

@tomchristie
Copy link
Member Author

Okay, perhaps we shouldn't take this approach?

I found it a useful as was thinking about how to structure the docs, and remaining places where we have potential for API improvements. An alternative might be to leave __all__ ordered, and instead make sure we've got a properly neat docstring that includes some kind of layout for everything that's exposed?

@zanieb
Copy link
Contributor

zanieb commented Oct 26, 2024

I don't have strong feelings, it's easy enough to revert in the future if we learn this ordering is hard to maintain. I just wanted to point out that automation exists. A docstring seems sensible too.

What value is there to ascribing contents to specific submodules?

There's also the option to do something like:

# Top level API.  `_api.py`
_api = [
    "delete",
    "get",
    "head",
    "options",
    "patch",
    "post",
    "put",
    "request",
    "stream",
]


# Authentication. `_auth.py`
_auth = [
    "Auth",
    "BasicAuth",
    "DigestAuth",
    "NetRCAuth",
]

__all__ = _api + _auth

or

from ._api import __all__ as __all__api
from ._auth import __all__ as __all__auth

__all__ = __all__api + __all__auth

or

__all__ = _api.__all__ + _auth.__all__

I'd need to double check but I'm pretty sure these formats are generally supported by type checkers and language servers.

@zanieb
Copy link
Contributor

zanieb commented Oct 26, 2024

There's also switching from __all__ to the redundant import alias. Then you can place commentary above the import and avoid embedding it into a big __all__ listing and importing * above

from ._api import (
    delete as delete,
    get as get,
    head as head,
    options as options,
    patch as patch,
    post as post,
    put as put,
    request as request,
    stream as stream,
)

There's a nice summary of the supported interface declarations in the Pyright docs

@AlexWaygood
Copy link

👋 Zanie asked for my thoughts here -- I authored the Ruff rule sorting __all__, and I also maintain the typing module at CPython.

Ruff supports sorting __all__ blocks — it's a bit different than this (and I'm not sure what happens if you create categories with comments), but it might be nice to automate.

Yeah, the Ruff rule tries very hard to preserve comments. But it doesn't understand using comments between __all__ items to create categories. So if you have a comment above (or on the same line as) an item in __all__, it will move the comment with that item to keep them together. But it will move __all__ items from one category into another (if you're using comments to create categories), and it will remove blank lines between the __all__ items.

In terms of the __all__ concatenations supported by type checkers, a comprehensive list of the concatenations that type checkers are expected to support according to the typing spec is here: https://typing.readthedocs.io/en/latest/spec/distributing.html#library-interface-public-and-private-symbols. However, not all type checkers are fully spec-compliant (and I believe on this specifically they may not all be fully spec-compliant), so you may need to do some experimentation with the type checkers you care about to check how they do. The Ruff rule PYI056 helps lint against __all__ operations that may be unsupported by type checkers (it's much more conservative than the list given in the spec, because there are several type checkers that aren't yet spec-compliant here).

There's also switching from __all__ to the redundant import alias.

Aesthetically my personal preference is to define __all__, but this also works fine!

@tomchristie
Copy link
Member Author

Thanks @zanieb, @AlexWaygood.

Not exactly obvious to me what'd be neatest here, so I'll just pass on this change for now.

@tomchristie tomchristie deleted the structure-imports branch October 28, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues and PRs related to code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up imports in __init.py__
3 participants