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

Django Timezone #36

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Django Timezone #36

wants to merge 6 commits into from

Conversation

camflan
Copy link

@camflan camflan commented Aug 3, 2018

Updated to use django.utils.timezone

@coveralls
Copy link

coveralls commented Aug 3, 2018

Coverage Status

Coverage increased (+14.5%) to 100.0% when pulling c81774a on camflan:master into 6bad475 on arteria:master.

@camflan
Copy link
Author

camflan commented Aug 3, 2018

This is ready for review @jvamvas @philippeowagner,

I think I handled timezones properly, trying only to convert to a location timezone where necessary for comparison.

@camflan
Copy link
Author

camflan commented Aug 4, 2018

Actually, I don't think I tackled this correctly. Ideally, we wouldn't override timezone.activate that might be set elsewhere in the codebase.

I've been thinking about how to properly fix it, and I think there are 2 possible ways:

  1. make OpeningHour boundaries aware by setting TZ on the datetime.time instances before they are saved to TimeField. Then we could use the aware (UTC) instances that timezone.now returns for comparisons.
  2. require passing location to get_now so we can create a location-local naive datetime for us to use for comparisons.

I think 1 is probably the best.

Also, can we get rid of the code in get_now that reaches into the request? I can't find anywhere that it's being used, and it would simplify that quite a bit.

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.

2 participants