[REFACTOR]: Enum constants by Bibo-Joshi · Pull Request #2708 · python-telegram-bot/python-telegram-bot

Bibo-Joshi · 2021-10-07T14:52:37Z

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:

Update references to telegram.ParseMode if Input media subclasses init handling refactor #2573 #2717 is merged before this
Add an enum for the InputMedia* types
Add enums for Message/attachment types

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

github-actions

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

harshil21

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

telegram/files/sticker.py

telegram/poll.py

telegram/constants.py

# 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 · 2021-10-10T15:33:27Z

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

harshil21

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

telegram/_message.py

telegram/ext/_updater.py

…ve_attachment & helpers.effective_message_type

Poolitzer · 2021-10-15T12:49:57Z

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 · 2021-10-15T13:30:17Z

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 …

harshil21

tiny fix

telegram/constants.py

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

# Conflicts: # telegram/_files/inputmedia.py # tests/test_constants.py

Bibo-Joshi · 2021-10-15T21:29:57Z

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 …

harshil21

another tiny unrelated change

telegram/_message.py

harshil21 · 2021-10-16T19:11:50Z

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

Poolitzer

Code changes look good!

docs/source/conf.py

Co-authored-by: Poolitzer <github@poolitzer.eu>

Bibo-Joshi added 3 commits October 6, 2021 23:37

Rework tg.constants with enums

6b910b7

Add slots and tests

816691a

more versioning directives

ed7206f

Bibo-Joshi added 🛠 refactor change type: refactor 🛠 breaking change type: breaking labels Oct 7, 2021

Bibo-Joshi added this to the v14 milestone Oct 7, 2021

github-actions bot reviewed Oct 7, 2021

View reviewed changes

Fix tests

a45cd9a

harshil21 requested changes Oct 8, 2021

View reviewed changes

harshil21 reviewed Oct 8, 2021

View reviewed changes

telegram/constants.py Outdated Show resolved Hide resolved

Bibo-Joshi mentioned this pull request Oct 9, 2021

Input media subclasses init handling refactor #2573 #2717

Merged

Bibo-Joshi added 3 commits October 10, 2021 15:52

review

c0147de

Merge branch 'v14' into enum-constants

1883de8

# Conflicts: # examples/errorhandlerbot.py # telegram/__init__.py # telegram/_chat.py # telegram/_chataction.py # telegram/_parsemode.py # telegram/_user.py # tests/test_bot.py

Replace a lot of hardcoded values in docstrings by links to constants

edd37d2

harshil21 reviewed Oct 10, 2021

View reviewed changes

telegram/_files/sticker.py Show resolved Hide resolved

telegram/_message.py Outdated Show resolved Hide resolved

telegram/ext/_updater.py Outdated Show resolved Hide resolved

Bibo-Joshi added 3 commits October 10, 2021 21:15

New InlineQueryLimit enum & some on-the-fly docfixes

f8274e5

New enums {InputMedia, InlineQueryResult}Type

7874998

Remove unnecessary workaround

586b65f

harshil21 approved these changes Oct 12, 2021

View reviewed changes

Bibo-Joshi added 2 commits October 12, 2021 18:10

New enums Message(Attachment)Type and some updates to Message.effecti…

2adae01

…ve_attachment & helpers.effective_message_type

Merge remote-tracking branch 'origin/enum-constants' into enum-constants

97501c3

harshil21 reviewed Oct 15, 2021

View reviewed changes

telegram/constants.py Outdated Show resolved Hide resolved

Bibo-Joshi and others added 4 commits October 15, 2021 18:15

fix typo

4df93c1

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

Improve references and add some enums to __all__

8042b7d

Merge branch 'v14' into enum-constants

7a1ffb7

# Conflicts: # telegram/_files/inputmedia.py # tests/test_constants.py

Fix tests

ba67755

Bibo-Joshi requested review from Poolitzer and harshil21 October 15, 2021 21:30

harshil21 reviewed Oct 16, 2021

View reviewed changes

telegram/_message.py Outdated Show resolved Hide resolved

Typos and const.LocationLimit

30917f1

harshil21 approved these changes Oct 18, 2021

View reviewed changes

Poolitzer approved these changes Oct 19, 2021

View reviewed changes

docs/source/conf.py Outdated Show resolved Hide resolved

Bibo-Joshi and others added 2 commits October 19, 2021 18:09

fix typo

511ef3f

Co-authored-by: Poolitzer <github@poolitzer.eu>

Update according to #2573

c72768b

Bibo-Joshi merged commit b24d7d8 into v14 Oct 19, 2021

Bibo-Joshi deleted the enum-constants branch October 19, 2021 16:28

github-actions bot locked and limited conversation to collaborators Oct 20, 2021

Conversation

Bibo-Joshi commented Oct 7, 2021 • edited by harshil21 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for PRs

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

harshil21 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bibo-Joshi commented Oct 10, 2021

Uh oh!

harshil21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Poolitzer commented Oct 15, 2021

Uh oh!

Bibo-Joshi commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harshil21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bibo-Joshi commented Oct 15, 2021

Uh oh!

harshil21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harshil21 commented Oct 16, 2021

Uh oh!

Poolitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bibo-Joshi commented Oct 7, 2021 •

edited by harshil21

Loading

harshil21 left a comment •

edited

Loading

Bibo-Joshi commented Oct 15, 2021 •

edited

Loading