Skip to content

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