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

API 4.7 #1858

Merged
merged 17 commits into from
Apr 10, 2020
Merged

API 4.7 #1858

merged 17 commits into from
Apr 10, 2020

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Mar 30, 2020

Closes #1856

Pure API updates:

  • Add class Dice
  • Add Bot.send_dice
  • Add Message.dice
  • Add Message.reply_dice
  • Add class BotCommand
  • Add Bot.get_my_commands
  • Add Bot.set_my_commands
  • Add Bot.commands property
  • Add parameter tgs_sticker to Bot.create_new_sticker_set
  • Add parameter tgs_sticker to Bot.add_sticker_to_set
  • Add StickerSet.thumb
  • Add Bot.set_sticker_set_thumb
  • Tests
  • Add warning about changed parameter order for Bot.create_new_sticker_set and Bot.add_sticker_to_set to release notes

New functionality:

  • Add Filters.dice to filter messages containing a dice
  • Allow Filters.dice(1) and Filters.dice([1, 5]) to filters for dices with specific values
  • Somehow allow to treat a dice message as text message with the corresponding emoji via Filters.text and Message.effective_message?
  • Tests

Problems I encountered:

  • I didn't have the means to nor did I feel like creating a custom animated sticker for testing purposes. So I just downloaded one vie TG desktop. Copyright concerns?
  • Bot.create_new_sticker_set was not tested before, I guess because we can't delete packs via the API. So I don't test with the new parameter, either.
  • Bot.upload_sticker_file seems to accept TGS files as png_sticker. I'm jusing it it test_bot_methods_1_tgs right now. Also pinged @botsupport about it. The resulting file_id is not valid
  • While improving coverage I noticed, that the mask_position for add_sticker_to_set was untested. If I see correctly, request posts differently when posting files, which is the case for stickers, and thus the mask_position has to be to_json-ed manually. Added that and the corresponding test.

@Bibo-Joshi Bibo-Joshi added the API label Mar 30, 2020
@Bibo-Joshi Bibo-Joshi added this to the 12.6 milestone Mar 30, 2020
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

Minor comments on the code, please excuse my short writing, I'm doing it from my phone.

Unitests are the most missed part here...

telegram/bot.py Outdated Show resolved Hide resolved
telegram/bot.py Outdated Show resolved Hide resolved
telegram/bot.py Outdated Show resolved Hide resolved
telegram/bot.py Show resolved Hide resolved
@Bibo-Joshi Bibo-Joshi mentioned this pull request Mar 30, 2020
4 tasks
@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review April 1, 2020 15:42
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

In addition to comments inline, please check the coverage report, I see some new code is not covered by tests.

telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/bot.py Show resolved Hide resolved
telegram/dice.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

we're getting close. a few minor details and we're set to go.

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Apr 8, 2020

we're getting close. a few minor details and we're set to go.

About the not-tested lines:

  • one chunk is Bot.create_new_sticker_set, which wasn't tested before (see PR desc)
  • the other is reply_markup in Bot.stop_poll which also wasn't tested before (introduced in All api 4.2 and 4.3 changes #1418 ). I test it now (at least I test, that passing a markup doesn't give errors. stop_poll returns only the Poll, not the complete message, so I can't check that the markup was actually updated.)

Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

Ok. We're down to only docstrings 🥳
Lets discuss them over chat and we can merge...

telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
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.

minor suggestions

telegram/bot.py Show resolved Hide resolved
telegram/bot.py Show resolved Hide resolved
telegram/bot.py Show resolved Hide resolved
telegram/bot.py Show resolved Hide resolved
telegram/botcommand.py Outdated Show resolved Hide resolved
telegram/dice.py Outdated Show resolved Hide resolved

dice = _Dice()
"""Dice Messages. If an integer or a list of integers is passed, it filters messages to only
allow those whose dice value is appearing in the given list.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
allow those whose dice value is appearing in the given list.
allow those with dice where the value is appearing in the given list.

Copy link
Member Author

Choose a reason for hiding this comment

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

While this is slightly more accurate, the sentence feels kinda weird to me. And it should be clear from the context, that only dice messages are allowed. I'll not change for now. If this is important to you, you can PR for it ;)

telegram/message.py Outdated Show resolved Hide resolved
telegram/message.py Outdated Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

CI fails unrelated. merging.

@Bibo-Joshi Bibo-Joshi merged commit d63e710 into master Apr 10, 2020
@Bibo-Joshi Bibo-Joshi deleted the api-4.7 branch April 10, 2020 17:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API 4.7
3 participants