Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add public variables to __all__ in main __init__.py #92

Merged
merged 1 commit into from
May 8, 2023

Conversation

thomasaarholt
Copy link
Contributor

Hiya,

I was poking around in pymc's typing, and came across this type error:
image

This is due to variables imported in __init__,py being "private" by default. The alternatives are either adding them under __all__ like I have done here (explicitly defining public variables) or importing them using as, for instance from .core import Backend as Backend. Since there are several variables, I chose the former.

You can read more in this documentation and this issue.

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

Thanks @thomasaarholt !

Is it okay to export things that are only optionally available? (clickhouse, ClickHouseBackend)

If not, the __all__ should maybe be moved up and appended in the try block?

@thomasaarholt
Copy link
Contributor Author

Good concern, but I've checked and we don't need to do that:

  • mypy doesn't complain about it
  • the entries in __all__ are strings and not the objects themselves, so that doesn't raise an error
  • I've tested this at runtime, and get an error when I try to explicitly import clickhouse, but not otherwise.

Screenshot 2023-05-08 at 12 40 59

@michaelosthege
Copy link
Member

Looks like pre-commit found some trailing whitespace

@thomasaarholt
Copy link
Contributor Author

Thanks! I squashed the commits, reset and ran precommit before re-committing. Should be good now.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants