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

Explicitly add all public ops.X names to __all__ in __init__.py #937

Merged
merged 9 commits into from
Jun 1, 2023

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented May 31, 2023

This is so that when using "import ops; ops.X" in a charm, Pyright and MyPy won't complain (in the charm's codebase).

For example, before this fix, running MyPy on a charm says this:

src/charm.py:14: error: Name "ops.CharmBase" is not defined  [name-defined]

And Pyright:

src/charm.py:32:50 - error: "CharmBase" is not exported from module "ops" (reportPrivateImportUsage)

This also avoids disabling the reportUnusedImport setting.

I was considering adding a test that checked that __all__ stays in sync with the imports below, but Pyright errors if we have a entry in one but not the other.

This PR also updates the docs generation to avoid the names appearing twice in the docs, once under ops.Foo and once under ops.submodule.Foo. Also a few other minor docs fixes.

This is so that when using "import ops; ops.X" in a charm, Pyright
and MyPy won't complain (in the charm's codebase).

For example, MyPy:

src/charm.py:14: error: Name "ops.CharmBase" is not defined  [name-defined]

And Pyright:

src/charm.py:32:50 - error: "CharmBase" is not exported from module "ops" (reportPrivateImportUsage)

This also avoids disabling the reportUnusedImport setting.

I was considering adding a test that checked that __all__ stays in sync
with the imports below, but Pyright errors if we have a entry in one
but not the other.
This makes things easier to find. If you want source order, read the
source.
@benhoyt
Copy link
Collaborator Author

benhoyt commented May 31, 2023

Unfortunately this will break existing anchor links to individual classes and methods in our RTD docs, but I don't see a workaround. (We might be able to inject custom JavaScript, but it's probably not worth it.) My intention is to search for existing #ops.submodule.Foo links in our SDK docs and replace them with #ops.Foo.

Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

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

I'm very much in favor of having the ecosystem tooling give us the right information about what attributes are available, and whether you are accessing public or private files. So any fix in that direction is good.
I think we have a small tweak to how we want to express that, which will be easier to maintain, but otherwise +1.

@@ -39,14 +39,129 @@
Full developer documentation is available at https://juju.is/docs/sdk.
"""

# The "from .X import Y" imports below don't explicitly tell Pyright (or MyPy)
# that those symbols are part of the public API, so we have to add __all__.
__all__ = [
Copy link
Member

Choose a reason for hiding this comment

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

In discussing this with Ben, he pointed me towards:
https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#library-interface

Which says that the other way of getting PyRight to consider these as Public interfaces is to do:

from .charm import (
  ActionEvent as ActionEvent
  ...
  )

We discussed whether this action of creating a local type alias is "more obvious" why you are doing it. And arguably it isn't a lot more obvious (but we have a caveat comment here to explain why we are duplicating __all__, so it isn't any more obvious, either).
However, the really big win is that ActionEvent as ActionEvent is 'local' and so when you want to add a new type that we have added to ops, you add 1 import line with both pieces and then it just works, rather than knowing that you need to import it in one place, and then reference that in all in another place. So it feels much easier to maintain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually agree with John's rationale here, and had a patch all ready to go -- it solves the issue with the type checking. However, Sphinx (our API docs generator), specifically its autodoc extension, doesn't pick up any names from ops as it still doesn't see those names as public.

You can have yet another explicit list of names in docs/index.rst, but that is error-prone and could easily get out of sync. There's probably a way to hack the Sphinx extension to be cleverer about this, but I've played around with it a bunch, and it doesn't seem to be easily configurable. So I don't think it's worth it.

Reverting back to using __all__ explicitly, which Sphinx understands fine.

Copy link
Collaborator Author

@benhoyt benhoyt Jun 1, 2023

Choose a reason for hiding this comment

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

A couple of other points regarding maintainability (in favour of __all__):

  • With the import X as X approach, it would be easy to misspell the alias without noticing, like from .model import Application as Appliction.
  • If you accidentally forget to add a name from the import list to the __all__ list, Pyright complains (Import "X" is not accessed). If you accidentally have an additional name in the __all__ list, again Pyright complains ("X" is specified in __all__ but is not present in module). This means CI will catch it if you add it in one place but not the other.


# This value selects if automatically documented members are sorted
# alphabetical (value 'alphabetical'), by member type (value
# 'groupwise') or by source order (value 'bysource'). The default is
# alphabetical.
autodoc_member_order = 'bysource'
autodoc_member_order = 'alphabetical'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes things easier to find. (If you want source order, you can read the source.)

@benhoyt benhoyt merged commit cde9900 into canonical:main Jun 1, 2023
@benhoyt benhoyt deleted the type-fixes branch June 1, 2023 03:53
@benhoyt benhoyt mentioned this pull request Jun 1, 2023
5 tasks
@benhoyt
Copy link
Collaborator Author

benhoyt commented Jun 1, 2023

I've just gone through our SDK docs and updated the ones linked in the menu tree at https://juju.is/docs/sdk, changing ops.submodule.X to ops.X (except for ops.pebble and ops.testing), and updating code snippets to the import ops style in several places. I think I got all the links, though I'm sure I missed some of the code style updates. Here's the list of pages I updated (a few might be missing from the list that I also updated):

I wish wish wish our docs were in source control. This would have been a much less painful process. I'd better look at that two-way Discourse sync soon...

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.

3 participants