LocalePreferences: bikeshed

[sffc]

LocalePreferences landed in #5729. Two things I want to decide about it before it gets shipped:

1. Name. Not a big fan of the name. These are not preferences for a locale. It is a locale associated with a preference bag. I think PreferencesLocale is a better name, but would also be happy with naming it "what it is" such as LanguageIdentifierWithRegionExtensions, for example.
2. Public fields. I don't consider the design of this type 100% finalized. The CLDR spec it is implementing is still a concept, not yet landed in UTS 35. I would prefer it to have private fields. EDIT: Moved to Discuss: Should LocalePreferences have public fields #5785

@zbraniecki @robertbastian

[sffc]

• @zbraniecki To reiterate my reasoning: I don't think there is value of this struct beyond ergonomics of internal operations. For me, it is almost the same as if we listed the field in the main preference bag. These are the "preferences related to locale". I would be opposed to PreferencesLocale because it conveys that this is a type of locale that has value as a standalone struct, but I see the only value is storage of fields and conversion to DataLocale.
• @sffc OK.
• @robertbastian I like having the type.

#[non_exhaustive]
pub struct ListFormatterPreferences {
    pub locale_prefs: LocalePreferences,
}

• @robertbastian The fields of ListFormatterPreferences should be private, right?
• @sffc You shouldn't ever convert a Locale to a LocalePreferences in user code, because you want to retain other Unicode extensions like numbering system. No strong opinion on the field being public overall
• @zbraniecki If we make them private, we can make them public later

Conclusion:

• For naming: keep LocalePreferences.
• Make the locale_prefs field of preferences bags private.

LGTM: @sffc @zbraniecki @robertbastian

[Manishearth]

Make the locale_prefs field of preferences bags private.

A quick attempt to do this doesn't work; the prefs_convert macro needs them. Should these be doc(hidden)? or perhaps private with a __get_locale_prefs() hidden method?

[Manishearth]

If we're making this private, we should probably document that these preferences are here, and have some form of way of setting them from the locale?

[sffc]

Now that we have prefs_convert, which didn't exist when we made the LGTM above, I'm a little bit less concerned about this field being public. However, I am concerned that LocalePreferences is marked as exhaustive:

#[allow(clippy::exhaustive_structs)] // this type is stable
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct LocalePreferences

[Manishearth]

@sffc Should I close #5955 then, or do you think this change is still valuable?

Overall having these types have all-public fields does feel nice from the POV of documentation, at least. When I made locale_prefs private I immediately felt like the Preferences types would need some documentation to replace that, since it's pretty common to understand types by looking at their fields from the docs.

However, I am concerned that LocalePreferences is marked as exhaustive:

This should be fixed by #5785 , yes? I can work on that next.

[sffc]

It's a bit confusing since these were both discussed on the same day, but given that we decided to make the fields of LocalePreferences be private, I think on this issue I might be of the opinion that locale_prefs should remain public (but maybe be renamed to locale_preferences???)

[Manishearth]

@zbraniecki @robertbastian what do you feel about the above proposal?

[zbraniecki]

I'm ok keeping it public

[Manishearth]

Current plan: rename the field to locale_preferences, move forward with making LocalePreferences internally private #5785