Add field set options constructor #5556
Conversation
sffc
requested review from
Manishearth,
zbraniecki and
nordzilla
as code owners
|
This isn't "completely done" but it should pass CI and I'd like a review before I add the remaining impls |
sffc
requested review from
robertbastian
and removed request for
zbraniecki and
nordzilla
| /// ); | ||
| /// ``` | ||
| #[cfg(feature = "compiled_data")] | ||
| pub fn try_new2(locale: &DataLocale, options: FSet) -> Result<Self, LoadError> |
There was a problem hiding this comment.
issue (nb): don't like this name, but we can revisit later
There was a problem hiding this comment.
I intend to replace the old try_new with the new signature
| input_day_of_week = $day_of_week_yesno:ident, | ||
| input_day_of_year = $day_of_year_yesno:ident, | ||
| input_any_calendar_kind = $any_calendar_kind_yesno:ident, | ||
| $(years = $years_yes:ident,)? |
There was a problem hiding this comment.
nit: document how the yesno stuff works for callers
| @@ -1365,72 +1365,75 @@ macro_rules! datetime_marker_helper { | |||
| (@years/typed, yes) => { | |||
| C::YearNamesV1Marker | |||
| }; | |||
| (@years/typed, no) => { | |||
| (@years/typed,) => { | |||
There was a problem hiding this comment.
nit: I have no idea what the no means, consider using something more descriptive than yes/no for this argument.
There was a problem hiding this comment.
I did add more docs yesterday, but not to this macro, which is two-layers internal. I'll keep adding docs as I refactor and clean up this file.
| /// Generates an impl of [`NeoGetField`] | ||
| macro_rules! impl_get_field { | ||
| ($type:path, never) => { | ||
| impl NeoGetField<NeverField> for $type { |
There was a problem hiding this comment.
Are you using NeoGetField for both fields (i.e. data) and options? I think that's confusing.
There was a problem hiding this comment.
Yes, it's using the same trait because the signature is what I need and it is reasonably close semantically. I am happy to rename the trait. As noted previously, I see the trait like AsRef except that it returns by value instead of by reference.
| /// use writeable::assert_try_writeable_eq; | ||
| /// | ||
| /// let formatter = | ||
| /// TypedNeoFormatter::try_new2( |
There was a problem hiding this comment.
I'd prefer if you just replaced the current constructors, then the docs would actually be a diff to what we have now. I have no idea what we have now.
There was a problem hiding this comment.
I think there are still some tests that need fixing.
I'm quite open to doing a gradual migration here. We have to take a holistic look at this crate in the end anyway.
#1317
#5269
This code now works. Notice the lack of spaceships.
The things formerly called "datetime markers" are now options bags. Example:
https://icu4x-pr-artifacts.storage.googleapis.com/gha/dd9ff1a4d063806210470658595a9055ddfe058d/rustdoc/icu_datetime/neo_marker/struct.NeoYearMonthMarker.html
Obviously I intend to change the names of things. I have not applied any of the naming conventions we've discussed. I'm only implementing the new constructor style that I think we agreed on in #5269.