Skip to content

Fix for tzinfo issues in JobQueue #1696

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
Merged
Bibo-Joshi merged 1 commit into from
Mar 30, 2020
Merged

Fix for tzinfo issues in JobQueue #1696

Bibo-Joshi merged 1 commit into from
Mar 30, 2020

Conversation

@Andrej730
Copy link
Contributor

@Andrej730 Andrej730 commented Jan 7, 2020 •
edited by Bibo-Joshi
Loading

Closes #1693

Changes:

  1. Added optional argument tzinfo for run_once, run_repeating.
  2. Made sure job.tzinfo will be set as UTC by default and not None.
  3. Added test that checks that all methods by default set job.tzinfo as UTC.

First I've added tzinfo argument to run_daily too but it's seems excessive. For example, to make sure that job will be ran every monday and tuesday on 14:00 in desired timezone you'd had to pass timezone twice - as time.tzinfo and as tzinfo.

target_tzinfo = dtm.timezone(dtm.timedelta(hours=3))
target_time = dtm.time(hour=14).replace(tzinfo=target_tzinfo)
job_queue.run_daily(
    job_callback,
    time=target_time,
    days=(0, 1),
    tzinfo=target_tzinfo)

And I think there is no need to use different timezones for time.tzinfo and tzinfo because result may be counterintuitive. So, I've removed tzinfo from run_daily.

@Andrej730 Andrej730 marked this pull request as ready for review January 7, 2020 12:12
@Andrej730 Andrej730 requested a review from Bibo-Joshi January 19, 2020 06:10
@Bibo-Joshi Bibo-Joshi added the 📋 pending-review work status: pending-review label Jan 19, 2020
Bibo-Joshi
Bibo-Joshi requested changes Jan 19, 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.

Hey. Thanks for the PR and sorry for the late reply.
I had a look at it and I think I changed my mind about the optional tzinfo argument:

  • For run_once, the Job.tzinfo isn't event accessed. We just run _put(job, when) and if when is naive, to_float_timestamp makes sure to interpret it as UTC
  • For run_repeating, only the first argument may need to be aware, so it holds the same as for run_once
  • For run_daily we indeed need the time zone but that can be extracted from time

So the issue is really to make sure that Job will have UTC tzinfo, when an naive object is passed. So the only change we really need is the one you made on line 403:

self.tzinfo = tzinfo or _UTC

I know that I said otherwise before but seeing the changes I believe that the additional optional argument is more confusing than helpful.
Would you mind reverting the other changes? The simplification in line 278 is okay, though and the additional test is good as well :)

Please tell, if I'm missing something in my argumentation.

@Bibo-Joshi Bibo-Joshi added this to the 12.5 milestone Feb 6, 2020
@Bibo-Joshi Bibo-Joshi added 📋 pending-reply work status: pending-reply and removed 📋 pending-review work status: pending-review labels Feb 6, 2020
@Andrej730
Copy link
Contributor Author

Andrej730 commented Feb 9, 2020

You're right in your argumentation - tzinfo is only really used in run_daily. My thought was to add tzinfo to all methods because it will be helpful for #1685 ( next_t uses Job.tzinfo to return datetime of the next job execution) - because after adding next_t property Job.tzinfo will be used more often so it will be convenient to provide option to define tzinfo during job initiation instead of:

job = job_queue.run_repeating(self.job_run_once, 10)
job.tzinfo = dtm.timezone(dtm.timedelta(hours=3))

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Feb 13, 2020

Okay, I see your point. However, I think it would be more appropriate then to move the addition of the tzinfo argument to #1685, because this is where it's acutally needed. Might seem very nitpicky, but a logical changelog often is crucial in debugging :D
Two more thoughts:

  • tzinfo should only be taking into account in the run_... methods, if there is no other way to pass it. E.g. for run_repeating, if first is passed as (date)time, this should be aware instead of passing an additional tzinfo. Please add corresponding checks that raise ValueErrors
  • We will also need test cases for the new argument. Both for the Job class and to make sure that the run_... methods use it correctly

@Andrej730 Andrej730 closed this Feb 22, 2020
@Andrej730 Andrej730 reopened this Feb 22, 2020
@Andrej730
Fix for tzinfo issues in JobQueue
4a6df22 
1. Made sure that default tzinfo in JobQueue is UTC python-telegram-bot#1693.
2. Added test that checks that all methods by default set job.tzinfo as UTC.
@Andrej730
Copy link
Contributor Author

Andrej730 commented Feb 22, 2020

Agree, I reverted changes except actual fix for #1693

@Andrej730 Andrej730 mentioned this pull request Feb 22, 2020
Merged
@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Feb 23, 2020

CI fail is unrelated

@Bibo-Joshi Bibo-Joshi added 📋 pending-merge work status: pending-merge bug 🐛 and removed 📋 pending-reply work status: pending-reply labels Feb 23, 2020
@Bibo-Joshi Bibo-Joshi removed the 📋 pending-merge work status: pending-merge label Mar 28, 2020
Poolitzer
Poolitzer approved these changes Mar 29, 2020
View reviewed changes
Copy link
Member

@Poolitzer Poolitzer left a comment •
edited
Loading

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 9cb34af into python-telegram-bot:master Mar 30, 2020
@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Mar 30, 2020

Thank you for you contribution!

@Bibo-Joshi Bibo-Joshi mentioned this pull request Apr 22, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2020
@Bibo-Joshi Bibo-Joshi added 🔌 bug pr description: bug and removed bug 🐛 labels Nov 3, 2024
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

🔌 bug pr description: bug

Projects

None yet

Milestone

12.6

Development

Successfully merging this pull request may close these issues.

[BUG] Job_queue is skipping job if timezone is not provided for job.run_daily

3 participants

@Andrej730 @Bibo-Joshi @Poolitzer