Rename _legacy to _adjust_to_cased #3826
Conversation
| @@ -146,16 +146,16 @@ impl CaseMapper { | |||
| /// as a `LanguageIdentifier` (usually the `id` field of the `Locale`) if available, or | |||
| /// `Default::default()` for the root locale. | |||
| /// | |||
| /// This function performs legacy head adjustment behavior when [`HeadAdjustment::Adjust`] is set. See | |||
| /// This function performs "adjust to cased" head adjustment behavior when [`HeadAdjustment::Adjust`] is set. See | |||
There was a problem hiding this comment.
"head adjustment" is confusing. I have never heard "head" used in discussion of titlecasing.
How about "titlecasing index adjustment" or similar?
There was a problem hiding this comment.
We've been calling this HeadAdjustment and TailCasing which I think has a nice symmetry. "Index adjustment" feels more internal, I think "head" is quite clear personally.
There was a problem hiding this comment.
Bikeshed:
- Head/Tail
- Lead/Trail
- Start/Remainder
| /// See struct docs on [`TitlecaseMapper`] for more information on head adjustment behavior and usage examples. | ||
| /// | ||
| /// ✨ *Enabled with the `compiled_data` Cargo feature.* | ||
| /// | ||
| /// [📚 Help choosing a constructor](icu_provider::constructors) | ||
| #[cfg(feature = "compiled_data")] | ||
| pub const fn new_legacy() -> Self { | ||
| pub const fn new_adjust_to_cased() -> Self { |
There was a problem hiding this comment.
Suggestion: on second thought, we don't really need this constructor. People can use CaseMapper::titlecase_segment_adjust_to_cased if they don't want to load the extra data.
There was a problem hiding this comment.
What if they want to still have it be the same type, though? Might make it clunky
There was a problem hiding this comment.
It's kind-of confusing that HeadAdjustment has 2 choices but there are 3 behaviors. We can add all 3 behaviors to that enum. Over in CaseMapper::titlecase_segment_adjust_to_cased, we decide what to do if given the enum variant which we don't support.
There was a problem hiding this comment.
yeah but the choice of behaviors affects data; we had kinda discussed this model and the 2x2 chart with the merged cells
There was a problem hiding this comment.
People who care about their data have a path; people who don't care as much about their data have a cleaner API. I think that's slightly better than a confusing API that affects everyone.
There was a problem hiding this comment.
Are you proposing we remove specifically the compiled_data adjust_to_cased ctor? That seems okay.
There was a problem hiding this comment.
What should we do over FFI, then?
|
Superseded by #3843 |
Uses @markusicu's suggestion from #3801 (fixes #3801).
Seems fine, though
adjust_to_cased_to_string()is kinda clunky.This is also the final open stabilization issue, so I'm going to mark this as fixing #3234 . (note for reviewers: if you think there are still outstanding items, feel free to edit the magic issue-closing comment out of this PR body)