core icon indicating copy to clipboard operation
core copied to clipboard

Add `datetime` platform

Open raman325 opened this issue 3 years ago • 13 comments

Proposed change

Adds a new datetime platform. See https://github.com/home-assistant/architecture/discussions/811 for summary. Related to https://github.com/home-assistant/core/pull/81948 and https://github.com/home-assistant/core/pull/81949

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [x] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • Link to frontend pull request: https://github.com/home-assistant/frontend/pull/14391
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/24988
  • Link to developer documentation pull request: https://github.com/home-assistant/developers.home-assistant/pull/1535

Checklist

  • [ ] The code change is tested and works locally.
  • [ ] Local tests pass. Your PR cannot be merged unless tests pass
  • [ ] There is no commented out code in this PR.
  • [ ] I have followed the development checklist
  • [ ] The code has been formatted using Black (black --fast homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [ ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

raman325 avatar Nov 11 '22 06:11 raman325

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (demo) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of demo can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign demo Removes the current integration label and assignees on the issue, add the integration domain after the command.

home-assistant[bot] avatar Nov 11 '22 06:11 home-assistant[bot]

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (input_datetime) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of input_datetime can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign input_datetime Removes the current integration label and assignees on the issue, add the integration domain after the command.

home-assistant[bot] avatar Nov 11 '22 06:11 home-assistant[bot]

Quick random thought – how does this deal with timezones and e.g. DST transitions, where a datetime may occur twice?

akx avatar Nov 11 '22 08:11 akx

I glanced over this one a bit, and one of the things I noticed was the absence of timezone handling? We should enforce/ensure a timezone is set/handled.

frenck avatar Nov 21 '22 21:11 frenck

I glanced over this one a bit, and one of the things I noticed was the absence of timezone handling? We should enforce/ensure a timezone is set/handled.

How do you propose we address that? On the state side, I could add a timezone attribute that pulls from the native_value if a timezone exists. Not sure about how the service to set the value would accept timezone though

raman325 avatar Nov 22 '22 05:11 raman325

On the state side, I could add a timezone attribute that pulls from the native_value if a timezone exists.

We should validate/ensure the provided returned datetime from native_value() has a timezone value set. We require the same for sensor entities with the timestamp device class. You could peek at SensorEntity::state()

Not sure about how the service to set the value would accept timezone though

Yup, same issue with the front end. I think there are multiple possible solutions. One could be having a timezone field, another would be requiring ISO8601 timestamps with timezone designators (which might probably be the safest option). ISO8601 does have documentation on how to behave when the timezone information is missing (it will assume local time in that case).

This also applies to the time entity.

frenck avatar Nov 22 '22 17:11 frenck

I don't know if my earlier question/thought got lost somewhere, but extra care should probably be taken to get the timezone stuff right.

Also, the timezone designator in ISO8601 doesn't convey information about time zones but about the offset from UTC when that time was recorded, so around DST transition time it'll make for a possibly confusing user experience.

Perhaps the TZ should be "local" (as in, instance-local, so it's localized according to DST rules for the instance's TZ), "UTC", or an offset?

EDIT: times in Python are not tz-aware, though, since the same time reoccurs every day around the planet anyway...

akx avatar Nov 22 '22 17:11 akx

EDIT: times in Python are not tz-aware, though, since the same time reoccurs every day around the planet anyway...

Right, but the time a third-party service provides can be from a different timezone than the HA instance (or the configured timezone in the instance) and can be different in the frontend as well. Thus, the fact a time "reoccurs every day around the planet anyway" is thus not something we can go with.

Anyways, that is a discussion for a different PR.

This PR should follow the existing specifications we already have for sensors (for its current values). The only remaining handling that needs care is service calls (which are also leveraged by the frontend).

frenck avatar Nov 22 '22 17:11 frenck

I don't know if my earlier question/thought got lost somewhere, but extra care should probably be taken to get the timezone stuff right.

Apologies, it didn't get lost, I just saw it a while ago and forgot about it!

raman325 avatar Nov 22 '22 18:11 raman325

I am unable to explain why the statistics tests are changing to fail now.

raman325 avatar Nov 23 '22 04:11 raman325

not stale

raman325 avatar Feb 20 '23 02:02 raman325

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar May 01 '23 11:05 home-assistant[bot]

Should it be added to https://github.com/home-assistant/core/blob/7b5d26d3faf537ba616894ec9d7a070afbb67a31/.core_files.yaml#L14 ?

epenet avatar May 01 '23 11:05 epenet