Skip to content

Add Defaults.run_async support #2210

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
merged 9 commits into from
Nov 17, 2020
Merged

Add Defaults.run_async support #2210

merged 9 commits into from
Nov 17, 2020

Conversation

starry-shivam
Copy link
Member

Adds run_async support in telegram.ext.Defaults as discussed in: #2208

@starry-shivam
Add Defaults.run_async support
dd409fd 
Signed-off-by: starry69 <starry369126@outlook.com>
Bibo-Joshi
Bibo-Joshi requested changes Nov 13, 2020
View reviewed changes
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

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!
Besides some nitpicks that I left, please make sure that pre-commit & codecov are happy. please see the contribution guide on how to run pre-commit locally. tests/test_dispatcher.py would be, where there new tests should go and you can take inspiration from the existing tests :)
we can ignore test_official for now, as we didn't merge API 5.0 yet …

@starry-shivam
Address some requested changes.
aaa8c60 
Signed-off-by: starry69 <starry369126@outlook.com>
@starry-shivam
Copy link
Member Author

Will look into pre-commit and tests for test/test_dispatcher.py tomorrow...

@starry-shivam
Add tests for defaults.run_async
82e2eae 
Signed-off-by: starry69 <starry369126@outlook.com>
@starry-shivam
Copy link
Member Author

@Bibo-Joshi is everything alright now?

Bibo-Joshi
Bibo-Joshi requested changes Nov 16, 2020
View reviewed changes
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

thanks for the update. apart from pre-commit not being happy, the test doesn't really make sense to me :/

@starry-shivam
Fix tests logic & add default value support for dp.add_error_handler
515771e 
Signed-off-by: starry69 <starry369126@outlook.com>
Bibo-Joshi
Bibo-Joshi requested changes Nov 16, 2020
View reviewed changes
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Please also add a test for the error handler similar to the othe new test. in pycharm you can easily run the tests with coverage, btw, so you can check for coverage before pushing.
and please run pre-commit locally (see contribution guide) ;)

@starry-shivam
Fix tests, with requested changes
fa6d391 
Signed-off-by: starry69 <starry369126@outlook.com>
@starry-shivam
Copy link
Member Author

starry-shivam commented Nov 17, 2020 •
edited
Loading

Yeah sure, I'll add test for err handler aswell... And yeah i did run pre-commit locally but dunno why mypy is failing here, as it was passing in my local environment

@Bibo-Joshi
Copy link
Member

dunno why mypy is failing here, as it was passing in my local environment

mypy isn't failing, black and flake8 are ;)

@starry-shivam
Add tests for error_handler
3434c4d 
Signed-off-by: starry69 <starry369126@outlook.com>
@starry-shivam
Copy link
Member Author

dunno why mypy is failing here, as it was passing in my local environment

mypy isn't failing, black and flake8 are ;)

Umm those were passing aswell ^_^
Screenshot_20201117-150426181-01

@starry-shivam
try to fix pre-commit
cf8127b 
Signed-off-by: starry69 <starry369126@outlook.com>
Bibo-Joshi
Bibo-Joshi requested changes Nov 17, 2020
View reviewed changes
Copy link
Member

@Bibo-Joshi Bibo-Joshi 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 close :) left some nitpicks and also black still fails. you can run it manually with black . before commiting, don't know why pre-commit doesn't fail for you …

@starry-shivam
Copy link
Member Author

Thanks for reviewing! I'll address suggested changes in evening..

@starry-shivam
Enhance tests & address suggested changes
6f7600a 
Signed-off-by: starry69 <starry369126@outlook.com>
@starry-shivam
Copy link
Member Author

@Bibo-Joshi is everything alright now?

Bibo-Joshi
Bibo-Joshi requested changes Nov 17, 2020
View reviewed changes
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

just one documentation nitpick left :) btw, github already notifies me on updates ;)

telegram/ext/defaults.py Outdated
@@ -43,6 +43,8 @@ class Defaults:
be ignored. Default: :obj:`True` in group chats and :obj:`False` in private chats.
tzinfo (:obj:`tzinfo`): A timezone to be used for all date(time) objects appearing
throughout PTB.
run_async (:obj:`bool`): Optional. Default setting for the ``run_async`` parameter of
handlers and error handlers registered through :meth:`Dispatcher.add_handler`.
Copy link
Member

Choose a reason for hiding this comment

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

please link both add_handler and add_error_handler at the right places ;)

@starry-shivam
Improve docs
bc3e830 
Signed-off-by: starry69 <starry369126@outlook.com>
@Bibo-Joshi Bibo-Joshi merged commit 425716f into python-telegram-bot:master Nov 17, 2020
@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Nov 17, 2020 •
edited
Loading

Thanks for your contribution and your patience :)

@starry-shivam starry-shivam deleted the Defaults.run_async branch November 18, 2020 02:43
@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2020
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
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
🔌 enhancement pr description: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@starry-shivam @Bibo-Joshi