Skip to content
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

[REFACTOR]: Enum constants #2708

Merged
merged 19 commits into from
Oct 19, 2021
Merged

[REFACTOR]: Enum constants #2708

merged 19 commits into from
Oct 19, 2021

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Oct 7, 2021

Hey. I'm aware that we didn't discuss this, but it felt like reasonable change. If you disagree, we can close :D
Key points of this PR:

  • group the constants in tg.constants into enums. This has the advantage of
    • better name space (ParseMode.HTML instead of PARSEMODE_HTML)
    • the enums are still instances of int/str, so no change there
    • the enums are iterable which may come in handy in some situations (see below)
    • we can control the representation of the enums (see below)
  • Some constants didn't really fit into groups, so I left them as standalone constants
  • For string enums, I made sure that the string representation reads something like <ParseMode.HTML>, rather than the default behavior of <ParseMode.HTML: 'html'>. This is because the actual value is of little interest and IMO it also encourages users to use the constants rather than plain strings.
  • tg.{ChatAction, ParseMode} were removed in favor of tg.constants.{ChatAction, ParseMode}.

Some more changes that could be made but that I didn't make yet because I wasn't sure if we want that:

  • For now I kept the constants like Update.ALL_TYPES. However, if you agree, I think we can remove them, as now one can just do list(constants.UpdateType).
  • In fact, one could think of dropping these constants alltogether and replacing e.g. Chat.{PRIVATE, CHANNEL, …} by Chat.TYPES = tg.constants.ChatType.

things that should be done in any case:

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests

Breaking changes:

  • Constants in the module constants are now enums

@Bibo-Joshi Bibo-Joshi added 🛠 refactor change type: refactor 🛠 breaking change type: breaking labels Oct 7, 2021
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Oct 7, 2021
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 :)

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.

Nice change. I thought of Enums too few months back, but dropped it for some reason.

Some changes to be made:

  • The ChatAction and ParseMode .rst files should be deleted, and from telegram.rst (sphinx raises warnings),
  • There's still telegram.ChatAction all over the docs, should be updated.

telegram/constants.py Outdated Show resolved Hide resolved
telegram/constants.py Outdated Show resolved Hide resolved
telegram/constants.py Outdated Show resolved Hide resolved
telegram/constants.py Outdated Show resolved Hide resolved
telegram/files/sticker.py Outdated Show resolved Hide resolved
telegram/poll.py Outdated Show resolved Hide resolved
telegram/constants.py Outdated Show resolved Hide resolved
# Conflicts:
#	examples/errorhandlerbot.py
#	telegram/__init__.py
#	telegram/_chat.py
#	telegram/_chataction.py
#	telegram/_parsemode.py
#	telegram/_user.py
#	tests/test_bot.py
@Bibo-Joshi
Copy link
Member Author

@harshil21 what do you think about the two points that I mentioned under "Some more changes that could be made but …"?

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.

what do you think about the two points that I mentioned under "Some more changes that could be made but …"?

I think we should keep both of them the way it is. Makes it slightly easier for the user. It's not immediately obvious that list(constants.UpdateType) is Update.ALL_TYPES

telegram/_files/sticker.py Outdated Show resolved Hide resolved
telegram/_message.py Outdated Show resolved Hide resolved
telegram/ext/_updater.py Outdated Show resolved Hide resolved
@Poolitzer
Copy link
Member

I like the organisation and improved import. I dont like the docs. Is there a way to get sphinx to insert the value of the constants we link to? I would combine this: Either inline link to the constant but write the value there. Or write the import phrase and insert the value in brakets.

I understand that we want to push people to use our constants, but if they dont want to for whatever reason, we take away the quick look up.

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Oct 15, 2021

Valid point @Poolitzer 👍 . A straightforward way would be to do :role:`title <target>`, which links to <target> but shows title. I'll try and check if we can somehow get sphinx to automatically infere the title from the <target> - we might need a custom :role: for that or something like this …

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.

tiny fix

telegram/constants.py Outdated Show resolved Hide resolved
Bibo-Joshi and others added 4 commits October 15, 2021 18:15
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
# Conflicts:
#	telegram/_files/inputmedia.py
#	tests/test_constants.py
@Bibo-Joshi
Copy link
Member Author

This was actually a bit easier than I thought :) I addded a new role :tg-const: and hooked into the sphinx build process so that :tg-const: links have the acutal value as title. see the changes in docs/source/conf.py for details. I also built the docs at RTD https://python-telegram-bot.readthedocs.io/en/enum-constants/ for easier viewing. The naming of :tg-const: is not fixed - I'm open to alternatives.

One thing that come to think of while doing this is that we also could change the type hints of the parameters where you should pass constants. e.g. parse_mode: constants.ParseMode = None tells your type checker that it's probably a bad idea to pass a plain string. But I wanted to hear your opinion on that and IMO that can also be done in a follow-up PR …

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.

another tiny unrelated change

telegram/_message.py Outdated Show resolved Hide resolved
@harshil21
Copy link
Member

parse_mode: constants.ParseMode = None tells your type checker that it's probably a bad idea to pass a plain string. But I wanted to hear your opinion on that

imo it should be as it is, because ultimately the value is actually a string, the user might think that ParseMode is some class they have to know how to initialize etc. Also whenever there's a new API update, and the user wants to use the new value, they'll have to switch back to a string anyway (until PTB updates).

Also skimming through that RTD I saw that we should add an Enum for location, see for e.g.:
https://python-telegram-bot.readthedocs.io/en/enum-constants/telegram.bot.html#telegram.Bot.edit_message_live_location

Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

Code changes look good!

docs/source/conf.py Outdated Show resolved Hide resolved
Bibo-Joshi and others added 2 commits October 19, 2021 18:09
Co-authored-by: Poolitzer <github@poolitzer.eu>
@Bibo-Joshi Bibo-Joshi merged commit b24d7d8 into v14 Oct 19, 2021
@Bibo-Joshi Bibo-Joshi deleted the enum-constants branch October 19, 2021 16:28
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants