Skip to content

Conversation

kencx
Copy link
Contributor

@kencx kencx commented Oct 1, 2021

Closes #2680

I also standardized some of the module imports, except in cases of circular imports.

Changes

  • Rename private modules in telegram package
  • Rename private modules in telegram.ext package
  • Rename telegram(.ext).utils to telegram(.ext)._utils

Todo

Bibo-Joshi and others added 19 commits September 15, 2021 17:09
…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.
@kencx kencx changed the title Private modules Rename private modules with leading underscore Oct 1, 2021
@Bibo-Joshi Bibo-Joshi marked this pull request as draft October 1, 2021 19:08
All mnodules have been renamed except telegram.ext.filters
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.

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.
Copy link

@github-actions github-actions bot left a 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 :)

Copy link

@github-actions github-actions bot left a 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 :)

Copy link

@github-actions github-actions bot left a 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 :)

@Bibo-Joshi Bibo-Joshi added the 📋 pending-reply work status: pending-reply label Oct 6, 2021
@kencx
Copy link
Contributor Author

kencx commented Oct 7, 2021

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 (ext)._utils.

For example, in BasePersistence , do we remove all mentions of the custom typing aliases, leaving just a description?

A proposed example:

abstract get_user_data()
    Will be called by telegram.ext.Dispatcher upon creation with a persistence object. It should return the user_data if stored.
    
    Returns
        The restored user data.
    Return type
        DefaultDict[int]

@Bibo-Joshi
Copy link
Member

Hi @Bibo-Joshi, I would be happy to update the documentation with the discussed changes

Sweet :) 👍

However, I am a little unsure of what you mean by removing all mentions of (ext)._utils.

Right, the persistence case is indeed a bit tricky. The point here is that in the usual case get_user_data should return a Dict[int, Dict], becaues context.user_data is usually of type Dict. However, if ContextTypes are used, then the it should return a Dict[int, context_types.user_data]. Maybe this can be formulated along the lines

abstract get_user_data()
    Will be called by telegram.ext.Dispatcher upon creation with a persistence object. It should return the user_data if stored, or an empty defaultdict. In the latter case, the defaultdict should produce values that correspond to the expected type of CallbackContext.user_data, which usually is simply dict unless ContextTypes are used.
    
    Returns
        The restored user data.
    Return type
        DefaultDict[int, Dict/Type specified by ContextTypes.user_data]

This is just a quick draft - feel free to reformulate :) A similar logic can be used for the other TypeVars in (.ext)._utils, i.e. describing which type is expected.
For type aliases (CDCData and ConversationDict), it should suffice to insert the actual type into the docs, replace ext._utils.ConversationDict with Dict[Tuple[:obj:`int`, ...], Optional[:obj:`object`]]

I hope this helps you get started :) If you have any questions, feel free to ping me here or on Telegram (@BiboJoshi)

@harshil21 harshil21 added this to the v14 milestone Oct 7, 2021
@harshil21 harshil21 added the 🛠 refactor change type: refactor label Oct 7, 2021
@kencx
Copy link
Contributor Author

kencx commented Oct 9, 2021

All mentions of tg.(ext)._utils modules should have been removed, but please let me know if I missed any.

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 :)

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.

Thanks for the updates! I left a few more nitpicks below :)

@kencx
Copy link
Contributor Author

kencx commented Oct 10, 2021

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?

@Bibo-Joshi Bibo-Joshi removed the 📋 pending-reply work status: pending-reply label Oct 10, 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.

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 :)

@Bibo-Joshi Bibo-Joshi mentioned this pull request Oct 10, 2021
Copy link
Member

@harshil21 harshil21 left a 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>
Copy link
Member

@harshil21 harshil21 left a 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
@Bibo-Joshi Bibo-Joshi merged commit eec90a0 into python-telegram-bot:v14 Oct 10, 2021
@Bibo-Joshi
Copy link
Member

Thank you for the contribution @kennethcheo :)

@github-actions github-actions bot locked and limited conversation to collaborators Oct 11, 2021
@harshil21 harshil21 added the hacktoberfest-accepted other: hacktoberfest-accepted label Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
hacktoberfest-accepted other: hacktoberfest-accepted 🛠 refactor change type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants