Open sffc opened 1 month ago
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.
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.
Would like comments from @robertbastian and @zbraniecki
I'm in favour of R::Options
for usability.
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> */)
NeoOptions<R>
YMD.with_length
or similar is better, we can do itYMD.with_length
because it goes out of the way to hide that the type has a generic.Constructor signature proposal:
Options<FSet>
with a field_set
fieldFSetOptions
, one type per field set markerDefault
and are inhabited ZSTs (have exactly 1 possible value)YMD.with_length(..)
etcFormatter::<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)LGTM: @Manishearth @sffc
New proposal:
Formatter::<YMD>::try_new(locale, Options {...})
is the primary constructorYMDOptions
, or just have YMD
be the options type), or a single options type generic over fieldsets (Options<YMD>
)
YMD.with_length(..)
convenience options builders which will produce an appropriately-constrained options typeFormatter::try_new(locale, YMD.with_length(..))
.with_length()
convenience ones in some docs where possible.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.
LGTM: @Manishearth @sffc
Need input from @robertbastian @zbraniecki
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
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.
This would produce good documentation, so I'm onboard.
Currently in #5248 I implemented
where
R::LengthOption
is set according to the field set. It is usuallyNeoSkeletonLength
but it is a zero-sized type for field sets that don't need the length.I intend to add at least two more options:
R::HourCycleOption
for runtime hour cycle choicesR::EraDisplayOption
for runtime era display choicesAnother approach would be to instead create separate options bags for each combinations of possible options. For example:
and then
R::Options
would choose the correct struct.Pros and cons:
NeoOptions<R>
makes it possible/easier to add more options in the future in a non-breaking wayNeoOptions<R>
is arguably more consistent with the neo formatter design which has aimed at lots of markers/traits but very few concrete typesR::Options
is probably more clear in documentation because you are constructing a real struct with specific fields, rather than a weird struct whose fields depend onR
I also wanted to raise the possibility of applying a similar treatment to the
format<I>(&self, input: &I)
function. Currently we have a trait that looks like this:which is blanked-implemented on all T satisfying its bounds. We could instead have
Pros and cons:
pub trait AllInputMarkers<R>
allows users to pass foreign types directly into the format function so long as they implement the traitpub struct Input<R>
makes the format function less generic (instead of being generic in bothR
andI
, now it is generic only inR
), probably resulting in smaller code sizepub struct Input<R>
allows clients to more easily drive the formatter even without usingicu_calendar::Date
or one of the other official date typesThoughts?
@Manishearth @robertbastian @zbraniecki