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

[Feature] add run_monthly() method #1705

Merged
merged 13 commits into from
May 2, 2020

Conversation

daviddl9
Copy link
Contributor

This PR exposes a run_monthly() method that allows users to schedule monthly jobs. This pr also closes #1697

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 PR @daviddl9. Please see my comments.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
tests/test_jobqueue.py Show resolved Hide resolved
tests/test_jobqueue.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!
Besides my comments, we'll also need a tests for day_is_strict. Should've mentioned earlier …

telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
telegram/ext/jobqueue.py Outdated Show resolved Hide resolved
tests/test_jobqueue.py Outdated Show resolved Hide resolved
tests/test_jobqueue.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 new update!
I have some minor comments left, but I will have to do another thorough review after them.

telegram/ext/jobqueue.py Show resolved Hide resolved
tests/test_jobqueue.py Outdated Show resolved Hide resolved
tests/test_jobqueue.py Outdated Show resolved Hide resolved
@daviddl9
Copy link
Contributor Author

thanks for your comments! have made the changes you requested

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

Hi. It seems one comment went missing from my last review. Sorry for that, since that's certainly my fault. I added it below.

The failing test is unrelated but we have a fix for than in master now. Could you merge master?

Thanks for putting up with my nitpicking ;)

telegram/ext/jobqueue.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.

Hi! Mostly looks good to me now, thanks for your patience :) I wanted to some final details (remove duplicate use of calendar.monthrange(next_year, next_month)[1] and calendar.monthrange(dt.year, dt.month)[1], but it seems you have disabled the option that allows me to do so. Would you mind checking the corresponding box (should be in the column to the right of the PR conversation at the very bottom)?

@daviddl9
Copy link
Contributor Author

Hi! Mostly looks good to me now, thanks for your patience :) I wanted to some final details (remove duplicate use of calendar.monthrange(next_year, next_month)[1] and calendar.monthrange(dt.year, dt.month)[1], but it seems you have disabled the option that allows me to do so. Would you mind checking the corresponding box (should be in the column to the right of the PR conversation at the very bottom)?

Hey Bibo, are you referring to the box that says "allow edit from maintainers"? if so, the box is already checked. If not, please let me know which box you want me to check

@Bibo-Joshi
Copy link
Member

Okay, I was just too stupid …

@Bibo-Joshi Bibo-Joshi added this to the 12.5 milestone Feb 23, 2020
@Bibo-Joshi
Copy link
Member

If #1685 is merged before this, the changes from there must be added

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.

LGTM

@Poolitzer
Copy link
Member

Just one quick question: Why is the urrlib3 submodule updated?

@Bibo-Joshi
Copy link
Member

Just one quick question: Why is the urrlib3 submodule updated?

Thanks for pointing it out!
Pretty sure that's a remant of #1775. I'll have to merge master and update according to #1685 anyway …

# Conflicts:
#	telegram/ext/jobqueue.py
#	tests/test_jobqueue.py
# Conflicts:
#	tests/test_jobqueue.py
@Bibo-Joshi Bibo-Joshi merged commit ae17ce9 into python-telegram-bot:master May 2, 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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] run_monthly method for job_queue
3 participants