Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

Use current time in location consistently #111

Merged
merged 1 commit into from
May 28, 2019

Conversation

grubbins
Copy link
Contributor

Should resolve #108

@grubbins
Copy link
Contributor Author

I have deployed the plugin with these changes, and it has fixed all the known incorrect results. So far everything is now working :)

Beyond this change, I think it would make sense to calculate time.Now().In(location) only once for each remind request and then propagate the result through all the rest of the calculations (maybe attach it to the request object?). Two reasons:

  1. Efficiency: we retrieve the location and invoke time.Now().In(location) multiple times for each reminder request
  2. Correctness: the result of time.Now().In(location) may change during the handling of the request, in which case we will get a broken result. We should perform all calculations with reference to the time of making the request.

However, that's a fair bit more work so maybe we could merge this PR and address the above changes separately?

@scottleedavis
Copy link
Owner

This is great, thank you @grubbins. 👍

I would be open to a separate PR to address 2 reasons you describe. Attaching the time.Now().In(location) to the request object makes sense. Aside from that, as long as the propagation of the new field doesn't cause much additional clutter, I am all for it.

@scottleedavis scottleedavis merged commit 10bd4ba into scottleedavis:master May 28, 2019
@scottleedavis
Copy link
Owner

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect behaviour in some cross-timezone cases
2 participants