Skip to content
This repository has been archived by the owner on Apr 23, 2024. It is now read-only.

Fixes timezone problem in execute_chore #51

Merged
merged 6 commits into from
Jan 14, 2020
Merged

Fixes timezone problem in execute_chore #51

merged 6 commits into from
Jan 14, 2020

Conversation

cerebrate
Copy link
Contributor

Description

The API call behind execute_chore expects either a UTC time or a local time including an appropriate time zone , whereas Python's datetime.now returns a local time without time zone information; this causes chore executions to be assigned the wrong time (for example, in CST (-6), all chore executions appear 6 hours into the future.)

The code in this pull request attaches the local timezone to the tracked_time before submitting it.

Related issue (if applicable): fixes issue 13 in lovelace-grocy-chores-card (bug found here)

Checklist

  • The code change is tested and works locally.
  • tests pass. Your PR won't be merged unless tests pass
  • There is no commented out code in this PR

@SebRut
Copy link
Owner

SebRut commented Jan 6, 2020

Thank you for your work, is there a way to achieve this without adding 2 more dependencies? Seems like this could be in the Std lib.

@cerebrate
Copy link
Contributor Author

To the best of my knowledge, there's only a partial one. It would be possible to use timezone , which is in the datetime module, to do the actual timezone correction, but it needs to know the time delta from UTC (i.e., timezone, daylight savings info, etc.).

AFAIK, the only way to get that information from the OS is tzlocal , which would pull in pytz anyway. There's a fairly ugly hack that can get the delta for the timezone alone, but that doesn't help with DST, etc., corrections throwing it off.

@SebRut
Copy link
Owner

SebRut commented Jan 7, 2020

Seems like travis flagged your PR as abuse, I'll have to check this.

pygrocy/grocy_api_client.py Outdated Show resolved Hide resolved
@SebRut
Copy link
Owner

SebRut commented Jan 7, 2020

Thank you, I contacted Travis CI about that abuse thing

@SebRut
Copy link
Owner

SebRut commented Jan 7, 2020

Travis issue should hopefully be resolved but we need another commit to trigger the build. Could you change something, e.g. remove the empty line added or add something to a comment please?

@cerebrate
Copy link
Contributor Author

I did that, and also fixed up the requirements, but Travis is still failing once it gets to the coverage run step. Not sure what the problem is.

requirements.txt Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@SebRut SebRut merged commit 61ef15b into SebRut:master Jan 14, 2020
@pull-assistant
Copy link

Score: 0.80

Best reviewed: commit by commit


Optimal code review plan

     Send chore execution time as local time.

     Updated setup accordingly.

     Don't shadow parameter.

     Removed blank line.

     Requirements.

     Requested changes.

Powered by Pull Assistant. Last update 8029d9d ... 23776de. Read the comment docs.

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.

2 participants