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

Dont floor duration for milliseconds #23028

Merged
merged 3 commits into from
Nov 27, 2024
Merged

Conversation

karwosts
Copy link
Contributor

Proposed change

Really like the new duration changes! However I was thinking maybe when a duration is set to milliseconds, we don't need to force round it.

If we don't floor, then the display is just controlled by display precision, so user can choose whole numbers or partial fraction.

If we floor, then there is no choice, and anything sub-1ms is always forced to 0 ms.

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

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

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:

@karwosts karwosts marked this pull request as ready for review November 27, 2024 14:54
Copy link
Member

@piitaya piitaya left a comment

Choose a reason for hiding this comment

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

I had to do that because the duration format only support integer.

I'm just thinking, may be we should just drop duration formatting for millisecond and only relies on regular numeric state formatting.
Just remove ms from this array and remove the ms case.

export const DURATION_UNITS = ["ms", "s", "min", "h", "d"] as const;

WDYT @karwosts ?

@karwosts
Copy link
Contributor Author

Hmm I guess I don't really understand what you mean when you say "duration format".

I tested this with some fractional ms and everything seemed to be working as expected. So I'm not sure what's not supported, (but I don't understand everything that's going on here).

If you think dropping the ms section achieves the same effect, we could try that too.

@piitaya
Copy link
Member

piitaya commented Nov 27, 2024

Using float for DurationFormat is not allowed.

new Intl.DurationFormat("en").format({ milliseconds: 0.1 })
VM60872:1 Uncaught RangeError: Invalid time value for Temporal ../../v8/src/objects/js-temporal-objects.cc:2574
    at DurationFormat.format (<anonymous>)
    at <anonymous>:1:31

It works in your code because if it's fail, we catch the error and we fallback to regular state formatting but I would prefer to remove ms from DURATIONS so we don't call the duration formatting for ms.

// state is duration
    if (
      attributes.device_class === "duration" &&
      attributes.unit_of_measurement &&
      DURATION_UNITS.includes(attributes.unit_of_measurement)
    ) {
      try {
        return formatDuration(
          locale,
          state,
          attributes.unit_of_measurement,
          entity?.display_precision
        );
      } catch (_err) {
        // fallback to default
      }
    }

@karwosts
Copy link
Contributor Author

Thank you for clarifying. Removing ms from DURATIONS seems to achieve the goal without any negative side effects as far as I can tell.

I have made the change here.

Copy link
Member

@piitaya piitaya left a comment

Choose a reason for hiding this comment

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

Thank you 👌

@piitaya piitaya enabled auto-merge (squash) November 27, 2024 17:26
@piitaya piitaya merged commit f7c8c6e into home-assistant:dev Nov 27, 2024
11 checks passed
@karwosts karwosts deleted the dont-floor-ms branch November 27, 2024 17:30
@silamon silamon added this to the 2024.12 milestone Nov 27, 2024
bramkragten pushed a commit that referenced this pull request Nov 28, 2024
* Dont floor duration for milliseconds

* remove ms
@bramkragten bramkragten mentioned this pull request Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants