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

chore: remove __all__ from non- __init__ files #406

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fstagni
Copy link
Contributor

@fstagni fstagni commented Mar 6, 2025

I do not see why they would be useful, but convince me otherwise.

@chrisburr
Copy link
Member

chrisburr commented Mar 6, 2025

__all__ lets us define the public API of each module.

@fstagni
Copy link
Contributor Author

fstagni commented Mar 6, 2025

__all__ lets us define the public API of each module.

It is only used when you do from module import *

@chrisburr
Copy link
Member

@fstagni
Copy link
Contributor Author

fstagni commented Mar 7, 2025

IMHO, that's not a particularly strong point.
I see few pros and some cons, mostly the added boilerplate. It's of very little use if we do not use import *, which we don't because is generally discouraged.

@chaen
Copy link
Contributor

chaen commented Mar 7, 2025

It's a way of making the public API explicit, and as @chrisburr said, it is used by autocomplete.
I am not in favor of forcing it to be there, but if it is there, I would not remove it

@aldbr
Copy link
Contributor

aldbr commented Mar 7, 2025

@fstagni created this PR because he noticed that JobParametersDB was not listed in __all__ within diracx.routers.dependencies. It means that we are forgetting to add new variables/functions in the public API over time, which might possibly create inconsistencies.

  • If there is an automatic rule (mypy?) checking that imported variables and functions are all part of __all__ if it exists, then we should probably go for it (though I haven't found anything by doing a quick search)
  • Else we should at least provide a piece of documentation about that for the developers (CODING_CONVENTION.md)

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.

4 participants