Neo date formatter: Options<R> vs R::Options and same for .format

[sffc]

Neo date formatter: Options vs R::Options and same for .format

from icu4x.

[Manishearth]

Discussed with Shane yesterday. In general I'm in favor of having the intermediate type with the user calling .into() or whatever, or having a single trait.

Both of these add some indirection, but they're more easily documented over a pile of traits. IMO functions like format() should spend ink documenting how they are used, without having to go too deep into "how to make the traits work". Having an intermediate type or trait gets us over the finish line with a dedicated place to describe and untangle the trait mess.

Between having an intermediate type or trait, I find that having a type tends to be less messy, and probably better for monomorphization.

[sffc]

In addition to hour cycle and era display, I also need an option for fractional second digits, unless that gets modeled as part of the field set.

[sffc]

Would like comments from @robertbastian and @zbraniecki

[robertbastian]

I'm in favour of R::Options for usability.

[sffc]

Discussion on constructors, also with implications on NeoOptions:

use icu::datetime::fieldsets::YMD;
use icu::datetime::DateComponents;

icu::datetime::Formatter::<YMD>::try_new(locale, options)
icu::datetime::Formatter::try_new_with_components(locale, DateComponents::YearMonth, options)

// Zibi prefers:
icu::datetime::Formatter::try_new(locale, YMD, options)

// Shane: It can also be written as:
icu::datetime::Formatter::<YMD>::try_new(locale, YMD, options)

// Robert: This would give gnarly errors
icu::datetime::Formatter::<YMDT>::try_new(locale, YMD, options)

// Robert: This actually works if the type can be inferred, i.e. it's declared in a field or argument
let formatter: Formatter<YMD> =
    icu::datetime::Formatter::try_new(locale, option)

// Shane: can we keep the old signature around?
icu::datetime::Formatter::<YMD>::try_new_bikeshed(locale, options)

// Shane: If we go with NeoOptions<R>, we could make it so that you write something like
icu::datetime::Formatter::try_new(locale, YMD.with_length(Length::Long) /* NeoOptions<YMD> */)

• @robertbastian You see these generics a lot. You can hide it, but you can't hide it when you pass it to a function, for example. And you see the type a lot in error messages.
• @zbraniecki For such a high-profile type, I don't think we necesarily want to force people to learn about how the generics work before they can even use it.
• @sffc We could add another constructor signature?
• @nekevss Could you put the field set into the options?
• @sffc Yes, if we go with NeoOptions<R>
• @robertbastian What do we use in docs?
• @sffc I slightly prefer using the turbofish in docs but if people think YMD.with_length or similar is better, we can do it
• @robertbastian I like fully specifying a type, but not a turbofish on function calls. I really don't like YMD.with_length because it goes out of the way to hide that the type has a generic.
• @Manishearth I don't think users need to understand they are different types.
• @sffc I think that Zibi's position about learnability is important. I think the type parameter is important for people to know about eventually, but not in their initial use. People are used to creating a DateTimeFormatter with fields specified as options: this is how Intl works, it is how ICU4C works and ICU4J and most other formatting libraries. So it makes sense to me that there should be a way to specify the field set as a parameter in the constructor.

Constructor signature proposal:

• The options type is one of the following:
  • Options<FSet> with a field_set field
  • FSetOptions, one type per field set marker
• All fieldsets implement Default and are inhabited ZSTs (have exactly 1 possible value)
• fieldset types gain convenience methods like YMD.with_length(..) etc
• We document three constructor call sites:
  • Formatter::<YMD>::try_new(locale, Options {...})
  • Formatter::try_new(locale, YMD.with_length(..))
  • Formatter::try_new(locale, Options { ..., fieldset: YMD }) (we don't need to display this in any docs, but people can do this if they want)
• In our docs we can choose to show different techniques if needed. We pick the first two techniques to primarily use in docs: the type param by default, and the helpers where possible.
• Auto-generated docs tests on marker types will use the first (type param) method.

LGTM: @Manishearth @sffc

New proposal:

• Formatter::<YMD>::try_new(locale, Options {...}) is the primary constructor
• If options design has one-type-per-fieldset (YMDOptions, or just have YMD be the options type), or a single options type generic over fieldsets (Options<YMD>)
  • We add YMD.with_length(..) convenience options builders which will produce an appropriately-constrained options type
  • We can also document Formatter::try_new(locale, YMD.with_length(..)).
  • We primarily still use the turbofish ctor, but use the with_length() convenience ones in some docs where possible.
• Else, if we have five options types DateOptions etc, they gain a generic and a field_set field, so you can say DateOptions {...field_set: YMD} if you choose, but also it can be defaulted as usual.
  • Downside: now we have generics on these options types and really don't need them usually. However, the choice of fieldset is something that can be considered an option so it's not too weird. (Check if @robertbastian is ok with this constraint)

LGTM: @Manishearth @sffc

Need input from @robertbastian @zbraniecki

[Manishearth]

The main thing that we discussed in the meeting after people left was the "else" block there, which proposes a way to decouple the ctor discussion from the options discussion, however it constrains the options discussion by giving DateOptions (etc) a generic in the "we have five options types" model

[sffc]

I want to explore the field set being the options type.

We would get call sites like this:

icu::datetime::Formatter::try_new(locale, YMD::with_length(Length::Medium))

The function signature is

impl Formatter<FSet> {
    pub fn try_new(Preferences, FSet)
}

And the field set would be defined like this (most likely expanded from a macro)

#[non_exhaustive]
pub struct YMD {
    pub length: Length,
    pub era_display: EraDisplay,
}

For runtime components, it would be

#[non_exhaustive]
pub struct DateSkeleton {
    pub field_set: DateFieldSet,
    pub length: Length,
    pub era_display: EraDisplay,
}

Assuming it works, I can agree to this.

[robertbastian]

This would produce good documentation, so I'm onboard.