Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

refactor: add enum constants#3351

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Bibo-Joshi merged 50 commits intomasterfromenums-instead-of-literals-3107
Nov 22, 2022
Merged

Conversation

lemontree210
Copy link
Member

@lemontree210lemontree210 commentedNov 8, 2022•
edited
Loading

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 inconstants.py:
    • add:paramref:s
  • add.. versionadded:: 20.0 to every added constant in "target" classes (e.g. like it's done for class constants inInlineQuery)

* 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 HEADINGconstant twice instead of PROXIMITY_ALERT_RADIUS(and hence 360 was displayed instead of 100000)
@lemontree210lemontree210 added the ⚙️ documentationaffected functionality: documentation labelNov 8, 2022
as suggested in review for PR#3336,but in singular (rather than `...Limits`)to conform with review for#3343
@lemontree210
Copy link
MemberAuthor

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

@lemontree210
Copy link
MemberAuthor

I don't think thatInlineKeyboardMarkupLimithere has actually something to do with limits for any params inBot.send_message.

BTW, classInlineKeyboardMarkupLimit inconstants.py doesn't seem to be used anywhere, not even inInlineKeyboardMarkup docstring.

@lemontree210

This comment was marked as resolved.

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

@lemontree210lemontree210 left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I remember that adding class constants toBotis not optimal, but shouldn't I fully implement the approachlaid out in description for#3107 inDice? Which would mean introducing class constants foremoji andvalue inDice class directly and linking them toconstants.py.

+ make sure the order of constants is kept in docs
Copy link
Member

@Bibo-JoshiBibo-Joshi left a comment

Choose a reason for hiding this comment

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

you're being thorough, nice! :)

note that this approach leads to similar constantsin multiple classes
reverting changes made to telegram.dice.rstincbe20e6
@lemontree210
Copy link
MemberAuthor

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

@lemontree210lemontree210 mentioned this pull requestNov 20, 2022
23 tasks
@lemontree210
Copy link
MemberAuthor

lemontree210 commentedNov 20, 2022•
edited
Loading

Savingcomments by@Bibo-Joshi here:

I vote to keepChatIniviteLinkLimit separate from a potentialChatLimit enum, sinceChatIniviteLinkLimit addresses limits of theChatInviteLink class rather than generic chat-related limits.

Putting title limits into aChatLimit enum is okay IMO.

I also vote to keep the currentChat* enums separate from aChatLimit 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 theChat class
  • ChatMemberStatus is about theChatMember class and not about limits
  • ChatType is not about limits

@lemontree210

This comment was marked as resolved.

@Bibo-Joshi

This comment was marked as resolved.

@lemontree210

This comment was marked as outdated.

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

@lemontree210lemontree210Nov 21, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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 (forBOWLING) 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 toDiceEmoji should also get the20.0 marking.

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

@lemontree210lemontree210 marked this pull request as ready for reviewNovember 21, 2022 16:59
@lemontree210lemontree210 self-assigned thisNov 21, 2022
Copy link
Member

@harshil21harshil21 left a comment

Choose a reason for hiding this comment

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

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

lemontree210 reacted with hooray emoji
@harshil21harshil21 linked an issueNov 21, 2022 that may beclosed by this pull request
@Bibo-JoshiBibo-Joshi merged commitc3f8fcd intomasterNov 22, 2022
@Bibo-JoshiBibo-Joshi deleted the enums-instead-of-literals-3107 branchNovember 22, 2022 10:07
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsNov 29, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@Bibo-JoshiBibo-JoshiBibo-Joshi left review comments

@harshil21harshil21harshil21 approved these changes

Assignees

@lemontree210lemontree210

Labels
⚙️ documentationaffected functionality: documentation
Projects
None yet
Milestone
v20.0a5
Development

Successfully merging this pull request may close these issues.

Constant improvement
3 participants
@lemontree210@Bibo-Joshi@harshil21
[8]ページ先頭
©2009-2025 Movatter.jp