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

Incorrect timezone #435

Merged
merged 4 commits into from Jul 2, 2023
Merged

Conversation

deathtenk
Copy link
Contributor

for issue: #413 , basically all this does is get the system timezone offset in seconds from time.timezone and subtracts it from expiry.timestamp. Also accounts for DST.

@gilesknap
Copy link
Owner

Thanks @deathtenk this looks plausible but seems awfully manual, I'm wondering if there is a library function to do this for us?

@juzna please could you comment on this?

@deathtenk
Copy link
Contributor Author

yea I only did it this way because it doesn't add a dependency and as a clojure developer I try to make it a habit of trying not to muck up dependency trees. My research on pythons core time and datetime stuff revealed its fairly limited in terms of what it can do at a high level. However I'm sure someone's written a library that would account for more edge cases (perhaps pytz?).

@gilesknap
Copy link
Owner

gilesknap commented Jun 15, 2023

Yes it looks like pytz does this kind of thing but as you say it feels odd to bring in another dependency for such a small thing.

I also find it strange that InstalledAppFlow is essentially giving us a UTC when the token requires local. It seems like the auth library is setting us up to fail. Or I still have not fully understood what is happening here.

@juzna you say that the auth library is doing the right thing here so do you think that @deathtenk 's fix is the way to go?

@gilesknap
Copy link
Owner

@deathtenk I'm going to run some tests with your changes. As I'm in British Summer Time right now your fix is relevant to me (in the winter GMT=UTC so the bug would not show up for me).

@juzna
Copy link
Contributor

juzna commented Jun 23, 2023

Hi, if you look at python documentation of the datetime.timestamp function used, it says exactly what to do:

There is no method to obtain the POSIX timestamp directly from a naive datetime instance representing UTC time. If your application uses this convention and your system timezone is not set to UTC, you can obtain the POSIX timestamp by supplying tzinfo=timezone.utc:
timestamp = dt.replace(tzinfo=timezone.utc).timestamp()

I didn't try to validate it with this library, but I think it should work.

@deathtenk
Copy link
Contributor Author

@gilesknap tested locally and this works. It's a more managed solution then the manual offset I was doing before, go ahead and test it on your end.

@gilesknap
Copy link
Owner

Thanks both will run some tests here in BST.

@gilesknap
Copy link
Owner

The test failures are due to httpbin.org being down. Not related to the code changes.

@gilesknap
Copy link
Owner

Testing looks good. merging...

Thanks @deathtenk and @juzna

@gilesknap gilesknap merged commit 8881b77 into gilesknap:main Jul 2, 2023
5 of 13 checks passed
@deathtenk deathtenk deleted the incorrect_timezone branch July 4, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants