-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Rename private modules with leading underscore #2687
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
Conversation
…ryptionError (python-telegram-bot#2621) * move telegramdecryptionerror to error.py * Change error class name
…t#2612) * feat: add docs about docs * fix: improve looks * fix: make link work * fix: this looks better * Improved markdown, updated link * Less justifying Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
* Fix incomplete type annotations for CallbackContext Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
* Feat: Custom pytest marker Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
* Make basepersistence methods abstractmethod Signed-off-by: starry69 <starry369126@outlook.com> Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
…m-bot#2634) Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Files that contain imported classes in `__init__` are considered private. This is in accordance with PEP8. The modules `chat, message, user, callbackquery, inline.inlinequery, passport.credentials, passport.passportdata, passport.passportfile, payment.precheckoutquery, payment.shippingquery` have not be renamed due to type checking issues.
Resolves type checking issues from previous commit.
All mnodules have been renamed except telegram.ext.filters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DeepSource fail is unrelated - should work on the next commit. Please make sure to merge v14 into your branch, since there are currently merge conflicts :)
telegram._{files, games, inline, passport, payment} are renamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Looks like you edited README.rst or README_RAW.rst. I'm just a friendly reminder to apply relevant changes to both of those files :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Looks like you edited the (dev) requirements or the pre-commit hooks. I'm just a friendly reminder to keep the pre-commit hook versions in sync with the dev requirements and the additional dependencies for the hooks in sync with the requirements :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there. Relax, I am just a little warning for the maintainers to release directly after merging your PR, otherwise we have broken examples and people might get confused :)
Hi @Bibo-Joshi, I would be happy to update the documentation with the discussed changes. However, I am a little unsure of what you mean by removing all mentions of For example, in BasePersistence , do we remove all mentions of the custom typing aliases, leaving just a description? A proposed example:
|
Sweet :) 👍
Right, the persistence case is indeed a bit tricky. The point here is that in the usual case
This is just a quick draft - feel free to reformulate :) A similar logic can be used for the other I hope this helps you get started :) If you have any questions, feel free to ping me here or on Telegram (@BiboJoshi) |
All mentions of The changes also follow the discussed scheme, and I hope I haven't altered any key phrasing or meanings. Let know me if anything could be worded better and I would be happy to edit it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! I left a few more nitpicks below :)
The past 4 commits have very similar messages as I didn't really know what to name them after the first one. Maybe we can squash them into 1 commit after all further documentation changes have been made? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm happy now :) THanks for your patience @kennethcheo! If @harshil21 gives a LGTM as well, we can rebase on v14, adapt to the changes of #2646 and merge :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last one
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
# Conflicts: # docs/source/telegram.ext.rst # telegram/ext/__init__.py # telegram/ext/_callbackcontext.py # telegram/ext/_contexttypes.py # telegram/ext/_dispatcher.py # telegram/ext/_updater.py # tests/test_dispatcher.py # tests/test_persistence.py # tests/test_updater.py
Thank you for the contribution @kennethcheo :) |
Closes #2680
I also standardized some of the module imports, except in cases of circular imports.
Changes
telegram
packagetelegram.ext
packagetelegram(.ext).utils
totelegram(.ext)._utils
Todo