Fix transliterator fallback loading #5468
Conversation
sffc
requested review from
robertbastian and
skius
and removed request for
a team
| let internal_t = dep | ||
| // a) hardcoded specials | ||
| .strip_prefix("x-") | ||
| .map(|special| Transliterator::load_special(special, normalizer_provider)) | ||
| // b) the user-provided override | ||
| .or_else(|| Transliterator::load_with_override(&dep, lookup?).transpose()) | ||
| // c) the data | ||
| .unwrap_or_else(|| { | ||
| let mut internal_t = None; | ||
| // a) hardcoded specials | ||
| if let Some(special) = dep.strip_prefix("x-") { | ||
| internal_t = Transliterator::load_special(special, normalizer_provider)?; | ||
| } | ||
| // b) the user-provided override | ||
| if let Some(lookup) = lookup { | ||
| if internal_t.is_none() { | ||
| if let Ok(locale) = dep.parse() { | ||
| internal_t = Transliterator::load_with_override(&locale, lookup)?; | ||
| } | ||
| } | ||
| } | ||
| // c) the data | ||
| let internal_t = match internal_t { | ||
| Some(t) => t, | ||
| None => { | ||
| let rbt = Transliterator::load_rbt( | ||
| #[allow(clippy::unwrap_used)] // infallible | ||
| DataMarkerAttributes::try_from_str(&dep.to_ascii_lowercase()).unwrap(), | ||
| transliterator_provider, | ||
| )?; | ||
| Ok(InternalTransliterator::RuleBased(rbt)) | ||
| })?; |
There was a problem hiding this comment.
I find this much more readable than the mutable Option you're using.
There was a problem hiding this comment.
Problems I had with the previous code:
- It was concealing the bug
- The functions involved return a mix of Option and Result and it is hard to keep track of which level you are at in unwrapping the values
- It's difficult to run the debugger with all the closures. You have to dip in and out of the Rust standard library with all the closures.
But I can convert this back to using the option chain if you prefer, as the code owner.
There was a problem hiding this comment.
The bug is fixed by having load_special return Option<Result<...>>. Here you only need to change the map to an and_then.
| @@ -341,7 +351,7 @@ impl Transliterator { | |||
| fn load_special<P>( | |||
| special: &str, | |||
| normalizer_provider: &P, | |||
| ) -> Result<InternalTransliterator, DataError> | |||
| ) -> Result<Option<InternalTransliterator>, DataError> | |||
There was a problem hiding this comment.
I think this should be Option<Result<InternalTransliterator, DataError>>. The option signals that a special transliterator is implemented, and then the result is the result from instantiating that transliterator, i.e. the _ case should just be None.
There was a problem hiding this comment.
Either way is fine except that Result<Option> means that I can use the ? operator.
There was a problem hiding this comment.
You don't need the ? opearator:
"any-nfkc" => Some(ComposingTransliterator::try_nfkc(normalizer_provider).map(InternalTransliterator::Composing)),
The "fix" is that |
The test that is fixed with this PR is in #5469. Which course of action do you prefer:
|
I don't know exactly what the code was doing, but pulling it out into pieces instead of chaining function calls makes it easier to reason about and debug.