8000 [BUG + REFACTOR] Change behavior of `JobQueue.run_monthly` · Issue #2627 · python-telegram-bot/python-telegram-bot · GitHub
[go: up one dir, main page]
Skip to content

[BUG + REFACTOR] Change behavior of JobQueue.run_monthly #2627

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
Closed
Bibo-Joshi opened this issue Aug 11, 2021 · 17 comments
Closed

[BUG + REFACTOR] Change behavior of JobQueue.run_monthly #2627

Bibo-Joshi opened this issue Aug 11, 2021 · 17 comments
Assignees
@iota-008
Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Milestone
v20.0a0

Comments

@Bibo-Joshi
Copy link
Member

Steps to reproduce

  1. job_queue.run_monthly(day=30, day_is_strict=False)

Expected behaviour

Runs on 30th day of each month excluding Feb + on Feb 28th/29th

Actual behaviour

Runs on 30th and 31st day of each month excluding Feb + on Feb 28th/29th

This happens because I didn't pay enough attention when writing this:

python-telegram-bot/telegram/ext/jobqueue.py

Lines 364 to 384 in 0c50850

else:
trigger = OrTrigger(
[
CronTrigger(
day=day,
hour=when.hour,
minute=when.minute,
second=when.second,
timezone=when.tzinfo,
**job_kwargs,
),
CronTrigger(
day='last',
hour=when.hour,
minute=when.minute,
second=when.second,
timezone=when.tzinfo or self.scheduler.timezone,
**job_kwargs,
),
]
)

However, the use case of day_is_strict is mainly "run on the last day of the month". I therefore suggest to just drop the parameter and instead allow to pass day='last' or day=-1 (I like this one better) to indicate that you want to run the job on the last day of the month.
The use case described above can still be achieved with run_custom.
@Bibo-Joshi Bibo-Joshi added bug 🐛 🛠 refactor change type: refactor 🛠 breaking change type: breaking labels Aug 11, 2021
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Aug 11, 2021
@iota-008
Copy link
Contributor

@Bibo-Joshi can i work on this issue?

@Bibo-Joshi
Copy link
Member Author

Hi, sure, you're welcome to! just make sure to base your branch on the v14 branch (and the PR should also go to there), as we're working towards a majorly breaking release :)

@iota-008
Copy link
Contributor

Hi, sure, you're welcome to! just make sure to base your branch on the v14 branch (and the PR should also go to there), as we're working towards a majorly breaking release :)

I have done the changes how can I test it?

@Bibo-Joshi
Copy link
Member Author

We use pytest for our unit tests. Please have a look at the contribution guide for details. Admittedly, unit tests are not always easy, so feel free to ping me if you get stuck - probably open a PR with your changes first though ;)

@iota-008
Copy link
Contributor

(on branch v14) i forked -> cloned -> changes -> pytest -> pre-commit -> added files -> commit -> pushed ( and it says every thing is upto date)
how to solve this?

@Poolitzer
Copy link
Member

It doesn't look like the commit actually commited. Maybe run git log -1?

@iota-008
Copy link
Contributor

It doesn't look like the commit actually commited. Maybe run git log -1?

yes, it's showing your commit but is that

@Bibo-Joshi
Copy link
Member Author
Bibo-Joshi commented Aug 19, 2021 •
edited
Loading

If it doesn't show your commit, then you haven't commited ;) double check the contribution guide and carefully read what's printed in the console on each of the steps you make. e.g. when you tried to commit, you may have overlooked a error message that git showed you. running git status can also be useful.

@Poolitzer Telegram GithubBot Revised
Copy link
Member

@iota-008 Or maybe you created your own branch, committed there, and switched back to V14 accidentally? That could be the case if git status doesn't yield changes

@iota-008
Copy link
Contributor

I am getting 3 assertion error's on pytest in test_schedule_removal, test_in_updater, in which I haven't changed anything.

@Poolitzer
Copy link
Member

Thats alright, job queue tests are very time sensitive. As long as they run on the CI, nothing to worry about. Pytest isn't part of the pre-commit hooks though and shouldn't stop your commit from going through

@iota-008
Copy link
Contributor
iota-008 commented Aug 19, 2021 •
edited
Loading

getting these errors now :(
telegram\passport\credentials.py:192: error: Item "Ed25519PrivateKey" of "Union[Ed25519PrivateKey, Ed448PrivateKey, RSAPrivateKey, DSAPrivateKey, EllipticCurvePrivateKey]" has no attribute "decrypt" [union-attr]

telegram\passport\credentials.py:192: error: Item "Ed448PrivateKey" of "Union[Ed25519PrivateKey, Ed448PrivateKey, RSAPrivateKey, DSAPrivateKey, EllipticCurvePrivateKey]" has no attribute "decrypt" [union-attr]

telegram\passport\credentials.py:192: error: Item "DSAPrivateKey" of "Union[Ed25519PrivateKey, Ed448PrivateKey, RSAPrivateKey, DSAPrivateKey, EllipticCurvePrivateKey]" has no attribute "decrypt" [union-attr]

telegram\passport\credentials.py:192: error: Item "EllipticCurvePrivateKey" of "Union[Ed25519PrivateKey, Ed448PrivateKey, RSAPrivateKey, DSAPrivateKey, EllipticCurvePrivateKey]" has
no attribute "decrypt" [union-attr]

telegram\passport\credentials.py:192: error: Incompatible types in assignment (expression has type "Union[Any, bytes]", variable has type "None") [assignment]

Found 5 errors in 1 file (checked 147 source files)

@Bibo-Joshi
Copy link
Member Author
Bibo-Joshi commented Aug 19, 2021 •
edited by Poolitzer
Loading

for now you can commit with get commit ... --no-verify. this skips the pre-commit checks. they run again in the CI anyway, so we'll see if there are any issues

@iota-008
Copy link
Contributor

i have made a pull request.

@iota-008
Copy link
Contributor

i have made a pull request

@Bibo-Joshi
Copy link
Member Author

I saw and I will review it when I have time for it 😉

@Bibo-Joshi Bibo-Joshi mentioned this issue Aug 20, 2021
Merged
3 tasks
@harshil21
Copy link
Member

GitHub's closing from PR comment feature is broken, so closing manually

@github-actions github-actions bot locked and limited conversation to collaborators Aug 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@iota-008 iota-008

Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Projects
None yet
Milestone
v20.0a0
Development

No branches or pull requests
4 participants
@Bibo-Joshi @Poolitzer @harshil21 @iota-008
0