Correct error types for icu_provider_fs
#3682
Conversation
|
Yes this requires 1.64 |
| @@ -10,6 +10,9 @@ use icu_provider::prelude::*; | |||
| /// | |||
| /// [`ForkByErrorProvider`]: super::ForkByErrorProvider | |||
| pub trait ForkByErrorPredicate { | |||
| /// The error to return if there are zero providers. | |||
| const ERROR: DataErrorKind = DataErrorKind::MissingDataKey; | |||
There was a problem hiding this comment.
Nit: NO_PROVIDER_ERROR_KIND or FALLBACK_ERROR_KIND would be more clear because ForkByErrorPredicate normally uses the predicate function to test its error type.
| let mut path_buf = self.root.clone().into_os_string(); | ||
| write!( | ||
| &mut path_buf, | ||
| "/{key}/{locale}.{}", |
There was a problem hiding this comment.
Issue, here and elsewhere: using / is not good because Windows and other OSes need different separators. That is what PathBuf::push handles for us. I don't think either write! or Path::new fixes it for us since write! is writing directly to a string and Path::new tries to borrow where possible.
There was a problem hiding this comment.
Apparently this works because Windows tests are not failing. Note that before we used push(&str), which used AsRef<Path> for &str, so the slashes in the str were still forward slashes (and push itself doesn't replace them).
There was a problem hiding this comment.
Okay, so before we were appending individual path segments for root, key, and locale, but the key can contain a slash which wasn't being processed.
I wouldn't be surprised if Windows accepts these characters but does some weird handling with them.
@eggrobin and @makotokato are the two ICU4X engineers who routinely use Windows and might be able to provide some insight here.
Blocked on #3680
Fixes #3610