Skip to content

chore: remove __all__ from non- __init__ files#406

Closed
fstagni wants to merge 1 commit intoDIRACGrid:mainfrom
fstagni:remove___all__
Closed

chore: remove __all__ from non- __init__ files#406
fstagni wants to merge 1 commit intoDIRACGrid:mainfrom
fstagni:remove___all__

Conversation

@fstagni
Copy link
Copy Markdown
Contributor

@fstagni fstagni commented Mar 6, 2025

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

@chrisburr
Copy link
Copy Markdown
Member

chrisburr commented Mar 6, 2025

__all__ lets us define the public API of each module.

@fstagni
Copy link
Copy Markdown
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
Copy Markdown
Member

@fstagni
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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