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

Round values for relative time instead of flooring #6225

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

gmta
Copy link
Contributor

@gmta gmta commented Jun 24, 2020

Proposed change

Currently, the value used for displaying a relative time is floored before it's interpolated into the localized string. Many times, this has led me to misinterpret the actual value:

  • 47 hours ago is displayed as "1 day ago" instead of "2 days ago"
  • 13 days in the future is displayed as "in 1 week"

This change modifies the relativeTime function to use Math.round instead of Math.floor so the output more closely matches the actual relative time of the input.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Just put any entity with device_class: timestamp on a Lovelace dashboard and fake the state to try out different edge cases. Unfortunately, I could not get a simple template with relative_time to work.

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

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

@homeassistant
Copy link
Contributor

Hi @gmta,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks !

@balloob
Copy link
Member

balloob commented Jun 26, 2020

Actually, we can't merge this unless there is also a PR to make sure the Python side is up to date: https://github.com/home-assistant/core/blob/dev/homeassistant/util/dt.py#L180 This version still uses rounding down.

@gmta
Copy link
Contributor Author

gmta commented Jun 26, 2020

Actually, we can't merge this unless there is also a PR to make sure the Python side is up to date

Makes sense, I'll look into it. Meanwhile I've updated this PR to get rid of the floor I erroneously introduced.

gmta pushed a commit to gmta/home-assistant-core that referenced this pull request Jun 26, 2020
gmta pushed a commit to gmta/home-assistant-core that referenced this pull request Jun 26, 2020
gmta pushed a commit to gmta/home-assistant-core that referenced this pull request Jun 26, 2020
gmta pushed a commit to gmta/home-assistant-core that referenced this pull request Jun 26, 2020
@gmta
Copy link
Contributor Author

gmta commented Jun 26, 2020

@balloob I've also created a PR for core: home-assistant/core#37125

@gmta gmta force-pushed the fix-relative-time-rounding branch 2 times, most recently from af0a169 to d73f1c3 Compare June 26, 2020 09:35
@gmta gmta requested a review from balloob June 26, 2020 09:35
@gmta gmta force-pushed the fix-relative-time-rounding branch from d73f1c3 to e06b23f Compare June 26, 2020 09:39
gmta pushed a commit to gmta/home-assistant-core that referenced this pull request Jun 26, 2020
gmta pushed a commit to gmta/home-assistant-core that referenced this pull request Jun 26, 2020
gmta pushed a commit to gmta/home-assistant-core that referenced this pull request Jun 26, 2020
@davet2001
Copy link

I have reviewed the HA core PR (looks good)

But it doesn’t (and didn’t previously) output ‘weeks’ that were mentioned in the description above.

It must be that 13days=1week was coming from somewhere else as I think it would now output:

13days=>13 days
14days=>14 days
15days=>15 days
16days=>1 month

@balloob balloob merged commit 8ce120b into home-assistant:dev Jul 9, 2020
@gmta
Copy link
Contributor Author

gmta commented Jul 10, 2020

@davet2001 The old code did send out "week" / "weeks", as can be seen here:

https://github.com/home-assistant/frontend/pull/6225/files#diff-94c31734a9546e7f680a38e3608c24eeL43

I've kept this behavior intact for the frontend:

https://github.com/home-assistant/frontend/pull/6225/files#diff-94c31734a9546e7f680a38e3608c24eeR24

This means that the new situation, just like the old one, means that frontend and core have different "fallback" time units of years and weeks, respectively.

@bramkragten bramkragten mentioned this pull request Jul 14, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants