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

warning to install optional dependencies #3393

Merged
merged 11 commits into from Dec 2, 2022

Conversation

clot27
Copy link
Member

@clot27 clot27 commented Nov 25, 2022

closes #3391

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.

The changes themself look got to me 👍

  • pre-commit is complaining a bit. If you run pre-commit install locally, the pre-commit hooks will run before you can commit
  • We'll need a test to ensure that the warning is issued as expected, i.e. the warning is there, the messages is the expected one and the warning points the user to the line where context.job_queue is accessed. You can e.g. have a look at TestJobQueue.test_run_daily for some inspiration on how that can be done :)

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

Some stylistic suggestions below.

Also the more likely scenario is when user does application.job_queue.run_* and faces an error. Shouldn't we make that a property to raise this warning as well?

docs/source/examples.rst Outdated Show resolved Hide resolved
docs/source/examples.rst Outdated Show resolved Hide resolved
docs/source/examples.rst Outdated Show resolved Hide resolved
examples/arbitrarycallbackdatabot.py Outdated Show resolved Hide resolved
examples/passportbot.py Outdated Show resolved Hide resolved
examples/timerbot.py Outdated Show resolved Hide resolved
telegram/ext/_callbackcontext.py Outdated Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member

Also the more likely scenario is when user does application.job_queue.run_* and faces an error. Shouldn't we make that a property to raise this warning as well?

I guess it's hard to say if app.job_queue or context.job_queue is used more … but anyway, making Application.job_queue a property and having it issue a warning just like CallbackContext.job_queue sounds like a nice idea :)

telegram/ext/_conversationhandler.py Outdated Show resolved Hide resolved
telegram/ext/_application.py Show resolved Hide resolved
telegram/ext/_callbackcontext.py Outdated Show resolved Hide resolved
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 updates!

  • TestConversationHandler.test_no_running_job_queue_warning currently fails b/c it's trying to override app.job_queue. Maybe it's best to build a new Application instance in that test, i.e. something like

    if not jq:
        app = ApplicationBuilder().token(bot.token).job_queue(None)
  • similar for TestConversationHandler.test_schedule_job_exception: here one would do AB().token(…).job_queue(DictJQ())

telegram/ext/_conversationhandler.py Outdated Show resolved Hide resolved
tests/test_application.py Outdated Show resolved Hide resolved
tests/test_callbackcontext.py Show resolved Hide resolved
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

okay I'm not sure why Github is not showing me the changes in conversationhandler.py in the files changed tab, but I think you should replace all instances of application.job_queue with application._job_queue to avoid unintentionally raising a warning in user code

tests/test_application.py Outdated Show resolved Hide resolved
tests/test_application.py Outdated Show resolved Hide resolved
tests/test_callbackcontext.py Show resolved Hide resolved
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.

I took the liberty to tie up the very last loose ends :) I'm happy now. If harshil is as well, we can merge.

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

LGTM

@Bibo-Joshi Bibo-Joshi merged commit f8be97c into master Dec 2, 2022
@Bibo-Joshi Bibo-Joshi deleted the raise_warning_when_jobqueue_is_none branch December 2, 2022 11:08
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CallbackContext.job_queue raise a warning when job_queue is None
4 participants