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

Fields Source of Truth #2

Closed
sffc opened this issue Jan 24, 2020 · 17 comments
Closed

Fields Source of Truth #2

sffc opened this issue Jan 24, 2020 · 17 comments
Labels
design Meeting Discussion Need to be discussed in one of the upcoming meetings

Comments

@sffc
Copy link
Collaborator

sffc commented Jan 24, 2020

Your draft API says,

const formatter =  new Intl.DurationFormatter('en-US' , {
  style: 'short',
  fields: 'hours-minutes-seconds'
});

const duration =  Temporal.Duration.from({
  hours: 2, minutes: 46, seconds: 40
});

console.log(formatter.format(duration));
// output: 2 hr 46 min 40 sec

In other words, you have fields from both the Temporal.Duration and the options bag. Why do you need them in both places?

It appears that Temporal.Duration will return 0 for a field that was not set in the constructor. Is it okay to hide all leading and trailing fields whose value is 0? So for example, if you do,

Temporal.Duration.from({ days: 0, hours: 12, minutes: 10, seconds: 0 })

then you get an hours+minutes format? Or are there cases in which you want to retain the zeroes in the leading and trailing fields?

@sffc
Copy link
Collaborator Author

sffc commented Jan 24, 2020

Maybe we could have an upper and lower field, like,

{
  lower: "second",
  upper: "hour"
}

Then, the hour, minute, and second are displayed, and all other fields from the Temporal.Duration are ignored. The default values would be

{
  lower: "second",
  upper: undefined
}

and the behavior would be to set "upper" to the largest nonzero field in the Temporal.DurationFormat. Thoughts?

@younies
Copy link
Member

younies commented Jan 30, 2020

Maybe we could have an upper and lower field, like,

{
  lower: "second",
  upper: "hour"
}

Then, the hour, minute, and second are displayed, and all other fields from the Temporal.Duration are ignored. The default values would be

{
  lower: "second",
  upper: undefined
}

and the behavior would be to set "upper" to the largest nonzero field in the Temporal.DurationFormat. Thoughts?

Using upper and lower fields is a great idea, because the user can specify any continuous set of fields to format the duration. However, this idea has the following problems:

    1. User can easily swap between lower and upper (because both of them belong to the same type (i.e. string in your comment).

@younies
Copy link
Member

younies commented Jan 30, 2020

then you get an hours+minutes format? Or are there cases in which you want to retain the zeroes in the leading and trailing fields?

Good point.

Actually, we need to add another two fields for hiding zero value fields and rounding the smallest field

  • Rounding: 1 hour, 2 minutes and 55.5 seconds --> 1 hour, 2 minutes and 56 seconds
  • Hiding zero value fields: 1 hour, 0 minutes and 50 seconds --> 1 hour and 50 seconds

Both of those options should be determine by the user (we can give them default values if the user did not set their values) because in some formatting widths (such as Narrow), some users do not prefer to hide the zero-valued fields. And in another formatting widths (such as Wide), some users prefer to hide the zero-valued fields.

@sffc sffc mentioned this issue Jan 31, 2020
@sffc
Copy link
Collaborator Author

sffc commented Jan 31, 2020

User can easily swap between lower and upper (because both of them belong to the same type (i.e. string in your comment).

I don't understand. Can you elaborate?

For hiding internal zeros: you can technically do this by formatting manually using Intl.NumberFormat with Intl.ListFormat.

var hf = new Intl.NumberFormat(undefined, { style: "unit", unit: "hour" });
var sf = new Intl.NumberFormat(undefined, { style: "unit", unit: "second" });
var lf = new Intl.ListFormat();
lf.format([hf.format(1), sf.format(50)])
// "1 hr and 50 sec"

The new API is first and foremost for handling the new Temporal.Duration type, and also to enable the numeric format in #1 and generally make this more discoverable and easier to use. Is hiding an internal zero a common enough case to put into the new API?

@younies
Copy link
Member

younies commented Apr 15, 2020

I think we will have two fields
{ from: "second", to: "hour" }

Therefore, the user can swap the fields without making errors

{ from: "hour", to: "second" }
is the same.

Also, the user can specify a start point or an endpoint

start point

{ from: "hour", to: undefined }

end point

{ from: undefined, to: "second" }

@sffc
Copy link
Collaborator Author

sffc commented Apr 15, 2020

Some discussion from the champions call today:

  1. What happens if your Temporal.Duration has nonzero days, hours, and minutes, and you have { from: "hour", to: undefined }? Do you show days & hours or hours & minutes?
  2. In Temporal difference methods, we use the terminology "largestUnit". Perhaps we should consider similar terminology: { smallestUnit: "second", largestUnit: "hour" }
  3. If the order of fields is wrong, like { smallestUnit: "hour", largestUnit: "second" }, then we can throw a RangeError.
  4. Consider following the model of RelativeTimeFormat in which we accept either singular or plural field names: "second" or "seconds".

@sffc
Copy link
Collaborator Author

sffc commented Apr 28, 2020

Related: tc39/proposal-temporal#533

@littledan
Copy link
Member

What was the motivation for adopting this smallestUnit/largestUnit option? For me, the dash-separated (or ideally, array, as in #3) seems both more intuitive/readable (I immediately know what it means when reading the code) as well as expressive (as it permits gaps). What were the disadvantages found with this model? Will there be too many fields, making it annoying to write them all out? (I hypothesize that the number of fields displayed will rarely be more than 4.)

@ryzokuken
Copy link
Member

So I talked with @younies today and reached the following conclusions:

  1. We will use the explicit "list of fields" approach for now in the spec.
  2. Younies would make it clear during the presentation that this is a not-very-ergonomic, mostly placeholder approach and discuss the most viable proposals (smallestUnit/largestUnit).
  3. We can make a final decision on how to best deal with this and update the proposal/spec before Stage 3.

@sffc @littledan do you agree?

@piuccio
Copy link

piuccio commented Jun 15, 2020

I understand the ergonomics of largestUnit but as mentioned before it doesn't allow gaps, which might be useful to exclude weeks.

Compare

const d = Temporal.Duration.from({days: 40}, {disambiguation: 'balance'});
new Intl.DurationFormat('en-US', {fields: ['month','day']}).format(d);
// ^ returns: 1 month and 10 days
new Intl.DurationFormat('en-US', {largestUnit: 'month', smallestUnit: 'day'}).format(d);
// ^ returns: 1 month, 1 week and 3 days

Surely both are fine but I imagine some people will want to use the first value which is not possible using smallest/largest.

@younies younies added design Meeting Discussion Need to be discussed in one of the upcoming meetings labels Jun 15, 2020
@ryzokuken
Copy link
Member

@piuccio that example is incorrect per-se because duration doesn't balance days to months or weeks. That said, I understand the concern about allowing to skip certain fields (for example hours and minutes in P1D42S). What would you think about allowing largestUnit and smallestUnit alongside an option to hide zero values?

@piuccio
Copy link

piuccio commented Jun 16, 2020

Oh I see, I was assuming that:

  1. Intl.DurationFormat would perform some arithmetic, but apparently that's not the case anymore as of Arithmetic: yes or no? #10
  2. the arithmetic would apply to week/month/year. ISO 8601 leaves the arithmetic up to applications and it's perfectly valid to assume P30D = P1M (obviously depends on the context)

In light of the decision to not support arithmetic, what should be the behavior of?

Intl.DurationFormat('en', {fields: ['month']}).format({minutes: 30});
// maybe throw a RangeError?
Intl.DurationFormat('en', {largestUnit: 'month', smallestUnit: 'day'}).format({hours: 23});
// is this:
// a) a RangeError too
// b) '0 days', or '1 day' when round:true

Ignoring for a moment the week/month/year problem, I actually think that largestUnit and smallestUnit is better because it avoids weird situations like:

fields: [hours,seconds], duration: 3690 s -> 1 h 90 s

which likely no one will prefer over 1 h 1 m 30 s.

@ryzokuken
Copy link
Member

ryzokuken commented Jun 16, 2020

@piuccio

Ignoring for a moment the week/month/year problem, I actually think that largestUnit and smallestUnit is better because it avoids weird situations ... which likely no one will prefer over 1 h 1 m 30 s.

Great point. It agree with this and also agree that cases like these make the largestUnit approach a naturally better choice for specifying fields (even though it's a little higher level).

In light of the decision to not support arithmetic, what should be the behavior of?

Currently, according the the spec, mismatches like these are ignored silently, however I agree that this is generally a bad idea. Once we finalize the decision in this thread, I'd be more than happy to discuss the issue you raised in more detail (since the choice we take here might very well affect our options).

We should discuss the three cases:

  1. df.[[Fields]] is a superset of Object.keys(duration.getFields()). (Never throw)
  2. df.[[Fields]] and Object.keys(duration.getFields()) have an overlap. (should this throw? dunno.)
  3. df.[[Fields]] and Object.keys(duration.getFields()) have no overlap. (IMO this should definitely throw)

@sffc
Copy link
Collaborator Author

sffc commented Jun 16, 2020

To lay out my prefered design for the option bag:

  • Have two options largestUnit and smallestUnit, which both default to undefined
  • When undefined, pick the largest and smallest units automatically based the largest and smallest nonzero units in the Temporal.Duration
  • Remove the fields and zeroValuedFields options

On the question of resolving a Temporal.Duration where the fields are out of bounds of the Intl.DurationFormat largest and smallest units: I think there are a few options.

  1. Display all nonzero fields, even when they are out of bounds.
  2. Filter out the out-of-bounds fields.
  3. Throw a RangeError.

Examples:

Largest - Smallest options Actual input fields Option 1 Option 2 Option 3
undefined - undefined minutes, days minutes, days (same) (same)
seconds - undefined minutes, days seconds, minutes, hours, days (same) (same)
seconds - minutes minutes, days seconds, minutes, days seconds, minutes throw
seconds - seconds minutes, days seconds, minutes, days seconds throw

Note that option 1 also has the side-effect of allowing you to hide zero-valued fields in the middle of the duration, for which @younies has repeatedly advocated. It would essentially make smallestUnit and largestUnit settings for where to start and end the zero-fill.

@piuccio
Copy link

piuccio commented Jun 17, 2020

Was there already a discussion on eliminating completely both fields and largest/smallestUnit and delegate everything to Temporal.Duration?

const d = Temporal.Duration.from({ seconds: 3640 });
const f = new Intl.DurationFormat('en-US');

f.format(d);
// ^ 3640 seconds
f.format(d.balance({ largestUnit: 'minute' }));
// ^ 60 minutes and 40 seconds
f.format(d.balance({ largestUnit: 'hour' }));
// ^ 1 hour and 40 seconds
f.format(d.balance({ largestUnit: 'hour', smallestUnit: 'minute' }));
// ^ 1 hour and 0.667 minutes
f.format(d.balance({ largestUnit: 'hour', smallestUnit: 'minute' }).round());
// ^ 1 hour and 1 minute
f.format(d.balance({ largestUnit: 'hour', smallestUnit: 'minute' }).getLargestUnit());
// ^ 1 hour
f.format(d.balance({ smallestUnit: 'day' }));
// ^ 0.04 days
f.format(d.balance({ smallestUnit: 'day' }).round()).format();
// ^ 0 days

// Note that we would still need an option for showing 0 values:
const withZero = new Intl.DurationFormat('en-US', { useZeroValues: true });
f.format(d.balance({ largestUnit: 'hour' }));
// ^ 1 hour, 0 minutes and 40 seconds

Basically having balancing and choosing units inside temporal duration, and duration formatter is simply converting a duration to plain text.

Arguably this approach resembles what's done in Intl.RelativeTimeFormat where the constructor only has styling options, while .format() takes a value and a unit. In our case Temporal.Duration is the combination of value and units.

It could also be argued that this approach removes all overlaps with Temporal.Duration (and a dependency on it) since all examples above can be converted with a plain object looking like {hour: 1}, {hour: 0.4, minute: 99} or whatever.

The .format() method can take any object with year, month, ... or a Temporal.Duration and call . getFields() on it, which returns an object.

The other advantage is that DurationFormat will never throw a RangeError which sounds counterintuitive when you try to format what otherwise is a perfectly valid duration.

@sffc
Copy link
Collaborator Author

sffc commented Jun 17, 2020

We decided in #10 to not have arithmetic in Intl.DurationFormat. Given that decision, the only real purpose for largestUnit/smallestUnit on Intl.DurationFormat is to control the display of zero-valued fields.

@younies
Copy link
Member

younies commented Aug 24, 2020

We narrow the discussion to the following issue #32

@younies younies closed this as completed Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Meeting Discussion Need to be discussed in one of the upcoming meetings
Projects
None yet
Development

No branches or pull requests

5 participants