-
Notifications
You must be signed in to change notification settings - Fork 5.7k
jobqueue monthly #2634
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
jobqueue monthly #2634
Conversation
There was a problem hiding this 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! I left a comment.
Apart from that, please have a look at the failing tests - the error messages should tell you what can be improved. If you can't see the error messages due to access stuff, please leave a comment.
Also, the tests need to be adapted. They are located at tests/test_jobqueue.py
. If you're having troubles with the unit tests or if you're new to pytest, feel free to ask for help :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I noticed that the docstring should be updated to state the changes. Please use the .. versionchanged::
directive (see here for an example) and explain in 2-3 sentences that
- the
day_is_strict
argument was removed - instead one can pass
-1
to theday
parameter to make the job run on the last day of the month.
Also update the docstring of the day
parameter
Checklist for PRs
.. versionadded:: version
,.. versionchanged:: version
or.. deprecated:: version
to the docstrings for user-facing changes (for methods/class descriptions, arguments and attributes)AUTHORS.rst
(optional)Closes #2627