Making IterableDataProvider #[no_std]
#5176
Conversation
| .then_with(|| self.locale.langid.total_cmp(&other.locale.langid)) | ||
| .then_with(|| self.locale.keywords.cmp(&other.locale.keywords)) |
There was a problem hiding this comment.
Issue: self.locale.langid and self.locale.keywords are private fields of DataLocale and they should not be being read by DataIdentifier; when we change DataLocale to have a different set of private fields, it should stay that way. I'm not sure why you removed DataLocale::total_cmp.
There was a problem hiding this comment.
DataIdentifier and DataLocale are very tightly coupled. I'm removing DataLocale::total_cmp because nobody should be putting a DataLocale in a BTreeMap, downstream code should always deal with DataIdentifiers.
There was a problem hiding this comment.
If the fields are private, it's not correct for code from a different struct to read them. You could make DataLocale::total_cmp be pub(crate) if you don't want people using it. But I won't block the PR on this.
There was a problem hiding this comment.
The fields are module-private, and are used from code in the same module.
There was a problem hiding this comment.
The types could be freely moved between module; I anticipate that DataLocale may make its way elsewhere, such as into icu_preferences. I would strongly prefer to keep the fields with struct-private behavior. It's not worth blocking the PR, but you've stated before, and I agree, that small issues should be resolved now instead of later.
| pub fn is_empty(&self) -> bool { | ||
| self == <&DataLocale>::default() | ||
| } |
There was a problem hiding this comment.
Nit: I think I prefer is_empty since that is what we use in Language and LanguageIdentifier. Also, is_und is ambiguous because does it return true or false for something like und-CH?
There was a problem hiding this comment.
It's called is_und on LanguageIdentifier. The language identifier is "und", it's not empty ("").
There was a problem hiding this comment.
I won't block the PR on this.
There was a problem hiding this comment.
Oh whoops. This is unreleased, and I think is_und is a better name, so I'd rather change LanguageIdentifier.
There was a problem hiding this comment.
This PR introduces an inconsistency which should be resolved before 2.0. I approved the LanguageIdentifier change a month or so ago because I think is_empty is the correct name. It sounds like we need to bikeshed.
| if provider.iter_ids_for_marker(marker)? != HashSet::from_iter([Default::default()]) | ||
| { | ||
| let supported = provider.iter_ids_for_marker(marker)?; | ||
| if supported.len() != 1 || !supported.first().unwrap().is_default() { |
There was a problem hiding this comment.
Nit: Write this in a way that avoids the unwrap. Something like
if !matches!((iter.next(), iter.next()), (Some(DataIdentifier::DEFAULT), None)) { ... }There was a problem hiding this comment.
I don't think your suggestion is an improvement.
There was a problem hiding this comment.
My suggestion is to remove the unwrap. I offered a one-liner to do so. I would normally block a PR on this but since it is in the export code I won't block on it.
By using
BTreeSetinstead ofHashSet. This removes thestdfeature fromicu_provider_adapters.Also removed
DataLocale::is_empty, which is the same asDataLocale::is_und, but "und" is a better description of that state.