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

Normative: Make DefaultNumberOption truncate before validating range #908

Closed

Conversation

ben-allen
Copy link
Contributor

Currently the behaviour of ECMA-402 differs from ECMA-262 in that 402's DefaultNumberOption validates that the value passed in is within the specified range before truncating that value, whereas 262's toFixed truncates before range validation. This PR changes 402 to align with 262. See #691, also tc39/proposal-temporal#2296

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Thanks!

@michaelficarra
Copy link
Member

From a normative perspective, this seems to go against our normative convention for rejecting non-integral arguments where we expect integers: https://github.com/tc39/how-we-work/blob/main/normative-conventions.md#integral-number-taking-inputs-should-reject-non-integral-arguments.

Why isn't this just throwing a RangeError for these invalid inputs?

@anba
Copy link
Contributor

anba commented Jul 25, 2024

From a normative perspective, this seems to go against our normative convention for rejecting non-integral arguments where we expect integers: https://github.com/tc39/how-we-work/blob/main/normative-conventions.md#integral-number-taking-inputs-should-reject-non-integral-arguments.

Does that mean Temporal should be changed to avoid ToIntegerWithTruncation?

Why isn't this just throwing a RangeError for these invalid inputs?

We can't really change DefaultNumberOption to throw for non-integer inputs, for example new Intl.NumberFormat("en", {minimumFractionDigits: 2.5}).format(1) currently works and returns "1.00".

This PR only allows two more possible inputs:

  • When zero is a valid input, x ∈ ]-1, 0[ is now additionally valid, too. Example: new Intl.NumberFormat("en", {minimumFractionDigits: -0.5}).format(1)
  • When max is the maximum valid integer, x ∈ ]max, max + 1[ is now valid, too. Example: new Intl.NumberFormat("en", {minimumFractionDigits: 100.5}).format(1)

Co-authored-by: Michael Ficarra <[email protected]>
@ben-allen ben-allen added the s: blocked Status: the issue is blocked on upstream label Aug 5, 2024
@ben-allen
Copy link
Contributor Author

I'll put in a link to the plenary's conclusions on this PR once the notes are up on GitHub, but in the meantime here's a summary:

  • We shouldn't change 402 to have the same behaviour as 262 in this case, since checking for this user error is more than valuable than consistency would be.
  • It's definitely not worth the effort to change the behaviour of 262 to match 402's current behaviour.
  • We'll investigate whether changing 402's behaviour to reject non-integers is worth the effort (I'm thinking no; see anba's comment above).
  • Future proposals should reject non-integers in similar cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus normative s: blocked Status: the issue is blocked on upstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants