Skip to content

Add Basketball Dice #1955

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.

Sign up for GitHub

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

Jump to bottom
Merged
Poolitzer merged 10 commits into from
Jun 4, 2020
Merged

Add Basketball Dice #1955

Poolitzer merged 10 commits into from
Jun 4, 2020

Conversation

@spontanurlaub
Copy link
Contributor

@spontanurlaub spontanurlaub commented May 14, 2020

There is a new Dice Variation for the Basketball (🏀) emote, that only returns values from 1-5.

@spontanurlaub
Add Basketball Dice
1ab7cdb 
Added Basketball Dice Variation
@spontanurlaub
Update dice.py
978dff3
Bibo-Joshi
Bibo-Joshi requested changes May 14, 2020
View reviewed changes
Copy link
Member

@Bibo-Joshi Bibo-Joshi 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.

Thanks for the quick PR! Unfortunately, there is no API release yet that officially confirms that the API handles the new basketball (though it does). We could still add the new functionality with corresponding notes that basketball is currently undocumented. In any case, we'd need:

  • Bot.send_dice: update docs for emoji
  • Create Filters.dice.basketball and update test_filters_dice_type in test_filters.py. Please see the contribution guide on how to run the tests.

Could you do that?

@Bibo-Joshi Bibo-Joshi added the ⚙️ bot-api affected functionality: bot-api label May 14, 2020
@spontanurlaub
Copy link
Contributor Author

spontanurlaub commented May 14, 2020

The requested changes are implemented now.

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented May 14, 2020

Thanks for the update. Still, after internal discussion we decided not to merge, before we get an official announcement from Telegram (that might i.e. even have info about the missing 6 for basketball).

@Bibo-Joshi Bibo-Joshi added the 📋 do-not-merge-yet work status: do-not-merge-yet label May 14, 2020
@tsnoam Telegram GithubBot Revised
Copy link
Member

tsnoam commented May 14, 2020

@Bibo-Joshi In general, we prefer not to merge undocumented features because they're prone to change.

@Poolitzer
Copy link
Member

Poolitzer commented Jun 4, 2020

Thanks for that, I will merge it in my 4.9 branch. Do you want to be included in the author file as well?

@Poolitzer Poolitzer changed the base branch from master to 4.9_update June 4, 2020 22:59
@Poolitzer Poolitzer merged commit 4b19c4f into python-telegram-bot:4.9_update Jun 4, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Bibo-Joshi Bibo-Joshi Bibo-Joshi requested changes

Assignees

No one assigned

Labels

⚙️ bot-api affected functionality: bot-api 📋 do-not-merge-yet work status: do-not-merge-yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@spontanurlaub @Bibo-Joshi @tsnoam @Poolitzer