refactor: add enum constants by lemontree210 · Pull Request #3351 · python-telegram-bot/python-telegram-bot

lemontree210 · 2022-11-08T11:02:09Z

Addresses #3107. This PR replaces #3336 that was a PR from my own fork. Instead, now it's a PR within the repository.

Among others, be sure to add limits for:
- set_chat_admin_custom_title
- set_chat_title
- set_chat_description
- create_new_sticker_set name, title
change docstrings in existing classes in constants.py:
- add :paramref:s
add .. versionadded:: 20.0 to every added constant in "target" classes (e.g. like it's done for class constants in InlineQuery)

* add constants for `max_connections` * expand docstring for `secret_token`

* add limits for heading * add minimum proximity_alert_radius (as it is non-zero) * fix docstrings that link to HEADING constant twice instead of PROXIMITY_ALERT_RADIUS (and hence 360 was displayed instead of 100000)

…terals-3107

as suggested in review for PR #3336, but in singular (rather than `...Limits`) to conform with review for #3343

lemontree210 · 2022-11-08T11:50:52Z

This is not part of the code I'm changing, but I was just creating a class with limits for ReplyKeyboardMarkup and noticed this. I don't think that InlineKeyboardMarkupLimit here has actually something to do with limits for any params in Bot.send_message, contrary to what's stated in the docstring.

lemontree210 · 2022-11-08T11:55:59Z

I don't think that InlineKeyboardMarkupLimit here has actually something to do with limits for any params in Bot.send_message.

BTW, class InlineKeyboardMarkupLimit in constants.py doesn't seem to be used anywhere, not even in InlineKeyboardMarkup docstring.

* add links to/from `InlineQueryResultLocation` and `InputLocationMessageContent` that were not added in previous commits * note that `Location` class does not have limits specified for `live_period` and `proximity_alert_radius` but `InlineQueryResultLocation` and `InputLocationMessageContent` do

…terals-3107

lemontree210

I remember that adding class constants to Bot is not optimal, but shouldn't I fully implement the approach laid out in description for #3107 in Dice? Which would mean introducing class constants for emoji and value in Dice class directly and linking them to constants.py.

+ make sure the order of constants is kept in docs

Bibo-Joshi

you're being thorough, nice! :)

docs/source/telegram.constants.rst

docs/source/telegram.dice.rst

telegram/_dice.py

telegram/constants.py

note that this approach leads to similar constants in multiple classes

Typo: "Minimum" instead of "Maximum"

reverting changes made to telegram.dice.rst in cbe20e6

telegram/_bot.py

lemontree210 · 2022-11-09T18:02:09Z

Do you think I should also replace "emoji literals" in filters.py with links to constants?

these are allowed lengths of strings, not allowed values

use existing class `BotCommandLimit`

minimum length is only specified in 1 class and 1 method

lemontree210 · 2022-11-20T16:20:32Z

Saving comments by @Bibo-Joshi here:

I vote to keep ChatIniviteLinkLimit separate from a potential ChatLimit enum, since ChatIniviteLinkLimit addresses limits of the ChatInviteLink class rather than generic chat-related limits.

Putting title limits into a ChatLimit enum is okay IMO.

I also vote to keep the current Chat* enums separate from a ChatLimit enum:

ChatAction is mostly related to send_chat_action and has little to do with the Chat class
ChatID also has little to do with the Chat class
ChatMemberStatus is about the ChatMember class and not about limits
ChatType is not about limits

* InlineQueryResultContact* InputContactMessageContent

lemontree210 · 2022-11-21T15:55:30Z

telegram/_dice.py

@@ -93,3 +106,45 @@ def __init__(self, value: int, emoji: str, *, api_kwargs: JSONDict = None):
    """


This comment is about the lines above this one. I didn't add any .. versionadded to class constants that I didn't create, though I think this needs to be done. I also think that the version marking in the line immediately above this one (for BOWLING) might be wrong: a bowling emoji was added in 13.4, but the constant itself is being introduced in 20.0. So maybe all constants referring to DiceEmoji should also get the 20.0 marking.

(Since I wasn't changing those lines, I couldn't add a comment to them directly)

to eliminate ambiguity

…terals-3107

harshil21

This PR is very high effort and simply insane. It was excruciating for me to just review over the past few days! Well done!

# Conflicts: # telegram/_bot.py # telegram/constants.py

lemontree210 added 9 commits November 5, 2022 14:01

refactor(Bot) add enum constants for Bot.set_webhook()

51023c9

* add constants for `max_connections` * expand docstring for `secret_token`

refactor(Bot) add enum constants for limit in Bot.get_updates()

38c7e75

refactor/fix(Bot) add enum constants to LocationLimit, fix docstrings

2014f32

* add limits for heading * add minimum proximity_alert_radius (as it is non-zero) * fix docstrings that link to HEADING constant twice instead of PROXIMITY_ALERT_RADIUS (and hence 360 was displayed instead of 100000)

refactor(Bot) add enum constants for live_period to LocationLimit

a167414

refactor(constants) paramrefs to constant docstr's (webhook, updates)

e2bd7f5

fix(constants) ref from constants to params (Location, send_location)

89e5b98

fix(constants) ref from constants to Bot.edit_message_live_location()

6b1ba23

refactor(Bot.get_user_profile_photos) add constants for limit

9f3c3ca

Merge remote-tracking branch 'origin/master' into enums-instead-of-li…

9de4b44

…terals-3107

lemontree210 added the ⚙️ documentation affected functionality: documentation label Nov 8, 2022

refactor: NumberOfUpdatesLimit -> PollingLimit

fcf6e57

as suggested in review for PR #3336, but in singular (rather than `...Limits`) to conform with review for #3343

lemontree210 mentioned this pull request Nov 8, 2022

refactor: add enum constants #3336

Closed

This comment was marked as resolved.

Sign in to view

lemontree210 added 6 commits November 8, 2022 15:44

feat: add ReplyKeyboardMarkupLimit

8a74b47

add :member-order: bysource to .rst file for constants.py

5667254

Merge remote-tracking branch 'origin/master' into enums-instead-of-li…

32d459f

…terals-3107

feat: new class DiceLimit

9fda7a7

refactor(Dice) replace emojis with links to constants in "Note:"

dbbbc9a

lemontree210 commented Nov 9, 2022

View reviewed changes

fix(Dice) create constants in class, link them to constants.py

cbe20e6

+ make sure the order of constants is kept in docs

Bibo-Joshi reviewed Nov 9, 2022

View reviewed changes

docs/source/telegram.constants.rst Outdated Show resolved Hide resolved

docs/source/telegram.dice.rst Outdated Show resolved Hide resolved

telegram/_dice.py Show resolved Hide resolved

telegram/_dice.py Outdated Show resolved Hide resolved

telegram/constants.py Show resolved Hide resolved

lemontree210 added 3 commits November 9, 2022 20:00

fix(Location etc) create constants in classes, link to constants.py

f1e0e4a

note that this approach leads to similar constants in multiple classes

fix(ReplyKeyboardMarkup) create constants in class, fix typo

69e9444

Typo: "Minimum" instead of "Maximum"

remove bysource sorting for Dice

93f5499

reverting changes made to telegram.dice.rst in cbe20e6

lemontree210 commented Nov 9, 2022

View reviewed changes

telegram/_bot.py Show resolved Hide resolved

lemontree210 commented Nov 9, 2022

View reviewed changes

telegram/_bot.py Outdated Show resolved Hide resolved

lemontree210 added 3 commits November 19, 2022 18:21

add limit for Bot.set_chat_administrator_custom_title()

1c1e906

add limit for Bot.set_chat_title()

0eca47c

add ChatDescriptionLimit, fix constants in TitleLimit

5850221

these are allowed lengths of strings, not allowed values

lemontree210 mentioned this pull request Nov 20, 2022

API 6.3 #3346

Merged

23 tasks

lemontree210 added 3 commits November 20, 2022 15:30

add limit for commands param of Bot.set_my_commands()

24dc038

use existing class `BotCommandLimit`

TEXT_LENGTH -> MAX_TEXT_LENGTH, add MIN_TEXT_LENGTH

687dd53

minimum length is only specified in 1 class and 1 method

add StickerLimit for Bot.create_new_sticker_set()

26eb73a

add constants to InlineQueryLimit (and InlineQuery)

72473e3

This comment was marked as resolved.

Sign in to view

lemontree210 added 3 commits November 21, 2022 13:34

add InlineQueryResultLimit for InlineQueryResult and subclasses

2235c84

SendContactLimit -> ContactLimit, add links from 2 classes

61d297f

* InlineQueryResultContact* InputContactMessageContent

add limit for InlineQuery.query to InlineQueryLimit

cb42bf1

This comment was marked as outdated.

Sign in to view

add .. versionadded:: 20.0 to constants in "target" classes

3df54ee

lemontree210 commented Nov 21, 2022

View reviewed changes

lemontree210 added 3 commits November 21, 2022 19:48

add references to attributes

07d59ca

constants.py: "length of"->"number of" +"characters"/"bytes"

390da52

to eliminate ambiguity

Merge remote-tracking branch 'origin/master' into enums-instead-of-li…

6f78556

…terals-3107

lemontree210 marked this pull request as ready for review November 21, 2022 16:59

lemontree210 self-assigned this Nov 21, 2022

harshil21 approved these changes Nov 21, 2022

View reviewed changes

harshil21 linked an issue Nov 21, 2022 that may be closed by this pull request

Constant improvement #3107

Closed

Bibo-Joshi added 2 commits November 22, 2022 10:45

Merge branch 'master' into enums-instead-of-literals-3107

50848bb

# Conflicts: # telegram/_bot.py # telegram/constants.py

Drop TitleLimit in favor of ChatLimit

44a51fb

Bibo-Joshi merged commit c3f8fcd into master Nov 22, 2022

Bibo-Joshi deleted the enums-instead-of-literals-3107 branch November 22, 2022 10:07

lemontree210 mentioned this pull request Nov 22, 2022

improvement of constants: unify ChatLimit #3377

Merged

github-actions bot locked and limited conversation to collaborators Nov 29, 2022

		@@ -93,3 +106,45 @@ def __init__(self, value: int, emoji: str, *, api_kwargs: JSONDict = None):
		"""

Conversation

lemontree210 commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lemontree210 commented Nov 8, 2022

Uh oh!

lemontree210 commented Nov 8, 2022

Uh oh!

This comment was marked as resolved.

lemontree210 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!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lemontree210 commented Nov 9, 2022

Uh oh!

lemontree210 commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

lemontree210 Nov 21, 2022 • 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!

harshil21 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lemontree210 commented Nov 8, 2022 •

edited

Loading

lemontree210 left a comment •

edited

Loading

lemontree210 commented Nov 20, 2022 •

edited

Loading

lemontree210 Nov 21, 2022 •

edited

Loading