Skip to content

Rearrange module level dunders and add public submodules #2805

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

Merged
merged 4 commits into from
Dec 5, 2021

Conversation

harshil21
Copy link
Member

Closes #2763

@harshil21 harshil21 added the misc label Dec 4, 2021
@harshil21 harshil21 added this to the v14 milestone Dec 4, 2021
@Bibo-Joshi
Copy link
Member

Changes look good 👍 What would you think about adding a unit test for this? i.e. check for every file that's named __init__.py or doesn't have an underscore in the path (like telegram/ext/filters.py) that

  • it has __all__
  • __all__ comes directly after the docstring
  • for inits, that all non-underscore modules of that directory are included in __all__

I guess that could be done with a bit of regex. E.g. for the second one something like ^(\#[^\n]*\n)*((\"{3}[^\"]*\"{3})|)([\n ]*)__all__ could work

Not sure if this actually helps or just makes more problems 😄

@harshil21
Copy link
Member Author

  • it has __all__

hm, I specifically removed __all__ for filters because iirc, it messed up with the docs (only rendered those in __all__) but maybe there's a setting for that.

  • __all__ comes directly after the docstring

actually I think this is something pylint should warn about. I'll open an issue upstream and see if they want to support it or not.

  • for inits, that all non-underscore modules of that directory are included in __all__

yes sounds reasonable.

@Bibo-Joshi
Copy link
Member

  • it has __all__

hm, I specifically removed __all__ for filters because iirc, it messed up with the docs (only rendered those in __all__) but maybe there's a setting for that.

to me that sounds like __all__ was missing a lot of stuff 😬

  • __all__ comes directly after the docstring

actually I think this is something pylint should warn about. I'll open an issue upstream and see if they want to support it or not.

nice idea 💪

@harshil21
Copy link
Member Author

all comes directly after the docstring

Opened pylint-dev/pylint#5471

@harshil21 harshil21 added the ⚙️ tests affected functionality: tests label Dec 4, 2021
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

nice 👍 about dunder names coming first: do you want to, wait for pylint, add a custom test temporarily or just merge? I'm open for all :)

@harshil21
Copy link
Member Author

about dunder names coming first: do you want to, wait for pylint, add a custom test temporarily or just merge? I'm open for all :)

I think we can merge and then just update pylint dependency if they support it.

@Bibo-Joshi Bibo-Joshi merged commit f0385cb into v14 Dec 5, 2021
@Bibo-Joshi Bibo-Joshi deleted the submodules-in-all branch December 5, 2021 08:42
Bibo-Joshi pushed a commit that referenced this pull request Dec 12, 2021
Bibo-Joshi pushed a commit that referenced this pull request Dec 12, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⚙️ tests affected functionality: tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants