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

Constructors should not accept invalid input #260

Closed
gibson042 opened this issue Nov 14, 2019 · 7 comments · Fixed by #405
Closed

Constructors should not accept invalid input #260

gibson042 opened this issue Nov 14, 2019 · 7 comments · Fixed by #405
Assignees
Labels
behavior Relating to behavior defined in the proposal has-consensus

Comments

@gibson042
Copy link
Collaborator

gibson042 commented Nov 14, 2019

I find it surprising (in a negative sense) that Date, Time, DateTime, YearMonth, MonthDay, and Duration constructors have disambiguation parameters, allowing for e.g. new Temporal.YearMonth(2019, 13) to succeed while Temporal.YearMonth.from("2019-13") rightfully fails. This also seems to have spread to the with<Type> methods (withTime, withDate, withYear, and withDay), which I thought were intended to losslessly preserve all data from the receiver and therefore have no use for disambiguation.

@gibson042 gibson042 added the behavior Relating to behavior defined in the proposal label Nov 14, 2019
@littledan
Copy link
Member

cc @ryzokuken , resident disambiguation expert. (Not knowing this area in detail, I might be OK with this being a bit stricter.)

@ryzokuken
Copy link
Member

@gibson042 the constructors handle invalid input through disambiguation params while the from helpers fail because the constructors are supposed to be lower level constructs that accept a wider range of inputs while ideally, the users would almost always stick with from. It's not much different the Node.js Buffer class where ideally you would always use from or alloc and never really the actual Buffer constructor.

@gibson042
Copy link
Collaborator Author

That seems to throw away many benefits that Temporal should have, some of which I believe have been cited as explicit improvements over Date. Why is new Temporal.Date(2019, 13, -9001) acceptable while new Date(2019, 13, -9001) is considered bad, and why should we permit Temporal.Date.from("2019-11-18").withTime({hour: 0, minute: 1e7}) to succeed under any circumstances?

@ptomato
Copy link
Collaborator

ptomato commented Jan 27, 2020

Meeting Jan. 27: Constructors should be strict, and we need to find another layer of the API to put the disambiguation.

@sffc sffc changed the title Why do constructors accept invalid input? Constructors should not accept invalid input Feb 13, 2020
@ptomato ptomato self-assigned this Feb 26, 2020
@ptomato
Copy link
Collaborator

ptomato commented Feb 27, 2020

Moving the disambiguation from the constructor into from() raises one question that I hadn't anticipated:

Temporal.Date.from({ year: 2019, month: 1, day: 32 }, { disambiguation: 'constrain' })  // => 2019-01-31
Temporal.Date.from('2019-01-32', { disambiguation: 'constrain' })  // throws

Since this is an invalid ISO 8601 string to begin with, this should throw. This seems confusing to API users who would expect that the constrain means they'll get a valid date back. But on the other hand, it doesn't seem right to disallow or ignore disambiguation when parsing a string, because this seems like it should be allowed, as in #388:

Temporal.Duration.from('PT100M', { disambiguation: 'balance' })  // => PT1H40M

The difference is that PT100M is a valid ISO 8601 string, but it does seem a bit confusing to an API user at first glance.

@gibson042
Copy link
Collaborator Author

This doesn't look terrible to me; it's just a reflection of the differences between strings (required to be valid per ISO 8601) and objects (inherently more flexible and in need of extra massaging).

@ptomato
Copy link
Collaborator

ptomato commented Feb 29, 2020

Fair enough. I pushed an extra commit to this branch making a note of this in the documentation, to clear up any potential confusion.

Ms2ger pushed a commit that referenced this issue Mar 2, 2020
Balance disambiguation, when creating a Duration, converts the duration
to the largest possible units, up to hours. (Not days, months, or
years.) Any days, months, and years supplied by the API caller are left
alone.

Note that this logic will be moved to from() in #260.

Closes: #388
Ms2ger pushed a commit that referenced this issue Mar 4, 2020
We have decided that constructors are to be strict (what was previously
achieved by passing 'reject' as the disambiguation parameter) and that
the disambiguation should become part of the from() methods.

Closes: #260.
ptomato added a commit that referenced this issue Nov 14, 2020
It was decided in #260 that overflow: 'constrain' should not affect
invalid values coming from ISO strings, but it seems that it crept back in
during a refactor. Fix this and add a test so that it doesn't regress
again.
ptomato added a commit that referenced this issue Nov 16, 2020
It was decided in #260 that overflow: 'constrain' should not affect
invalid values coming from ISO strings, but it seems that it crept back in
during a refactor. Fix this and add a test so that it doesn't regress
again.
ptomato added a commit that referenced this issue Nov 16, 2020
It was decided in #260 that overflow: 'constrain' should not affect
invalid values coming from ISO strings, but it seems that it crept back in
during a refactor. Fix this and add a test so that it doesn't regress
again.
ptomato added a commit that referenced this issue Nov 16, 2020
It was decided in #260 that overflow: 'constrain' should not affect
invalid values coming from ISO strings, but it seems that it crept back in
during a refactor. Fix this and add a test so that it doesn't regress
again.
ptomato added a commit that referenced this issue Nov 16, 2020
It was decided in #260 that overflow: 'constrain' should not affect
invalid values coming from ISO strings, but it seems that it crept back in
during a refactor. Fix this and add a test so that it doesn't regress
again.
ptomato added a commit that referenced this issue Nov 16, 2020
It was decided in #260 that overflow: 'constrain' should not affect
invalid values coming from ISO strings, but it seems that it crept back in
during a refactor. Fix this and add a test so that it doesn't regress
again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal has-consensus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants