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.

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

[Feature] add run_monthly() method #1705

Merged
merged 13 commits into from
May 2, 2020
Merged

[Feature] add run_monthly() method #1705

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

Bibo-Joshi
Bibo-Joshi requested changes Jan 14, 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 PR @daviddl9. Please see my comments.

@daviddl9 daviddl9 requested a review from Bibo-Joshi January 18, 2020 01:48
Bibo-Joshi
Bibo-Joshi requested changes Jan 18, 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 updates!
Besides my comments, we'll also need a tests for day_is_strict. Should've mentioned earlier …

@daviddl9 daviddl9 requested a review from Bibo-Joshi January 18, 2020 19:46
@Bibo-Joshi Bibo-Joshi added the 📋 pending-review work status: pending-review label Jan 19, 2020
Bibo-Joshi
Bibo-Joshi requested changes Jan 21, 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 new update!
I have some minor comments left, but I will have to do another thorough review after them.

@daviddl9
Copy link
Contributor Author

thanks for your comments! have made the changes you requested

@daviddl9 daviddl9 requested a review from Bibo-Joshi January 27, 2020 18:39
Bibo-Joshi
Bibo-Joshi requested changes Jan 28, 2020
View reviewed changes
@daviddl9 daviddl9 requested a review from Bibo-Joshi February 5, 2020 06:30
Bibo-Joshi
Bibo-Joshi requested changes Feb 5, 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.

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 ;)

@daviddl9 daviddl9 requested a review from Bibo-Joshi February 8, 2020 22:44
Bibo-Joshi
Bibo-Joshi reviewed Feb 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.

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

@daviddl9 daviddl9 requested a review from Bibo-Joshi February 19, 2020 21:41
@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 Bibo-Joshi removed the 📋 pending-review work status: pending-review label Feb 23, 2020
@Bibo-Joshi
Copy link
Member

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

Poolitzer
Poolitzer approved these changes May 1, 2020
View reviewed changes
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 …

Bibo-Joshi added 3 commits May 1, 2020 15:16
@Bibo-Joshi
Merge branch 'master' into monthy-job
2c7a7a9 
# Conflicts:
#	telegram/ext/jobqueue.py
#	tests/test_jobqueue.py
@Bibo-Joshi
Update according to python-telegram-bot#1685, minor robustness changes
95c758d
@Bibo-Joshi
Merge branch 'master' into monthy-job
130dc7f 
# 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.
Reviewers

@Bibo-Joshi Bibo-Joshi Bibo-Joshi approved these changes

@Poolitzer Poolitzer Poolitzer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
12.6
Development

Successfully merging this pull request may close these issues.

[FEATURE] run_monthly method for job_queue
3 participants
@daviddl9 @Bibo-Joshi @Poolitzer