Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be more consistent about options modules #5991

Open
sffc opened this issue Jan 14, 2025 · 2 comments
Open

Be more consistent about options modules #5991

sffc opened this issue Jan 14, 2025 · 2 comments
Labels
2.0-breaking Changes that are breaking API changes C-meta Component: Relating to ICU4X as a whole

Comments

@sffc
Copy link
Member

sffc commented Jan 14, 2025

icu_datetime has an options module which is public.

icu_collator and icu_segmenter have a private options module and re-export the things from the top level.

Haven't done an audit of the other crates.

Should we be more consistent?

@zbraniecki @Manishearth

@sffc sffc added the discuss Discuss at a future ICU4X-SC meeting label Jan 14, 2025
@sffc sffc added this to the ICU4X 2.0 Stretch ⟨P2⟩ milestone Jan 14, 2025
@Manishearth
Copy link
Member

Manishearth commented Jan 14, 2025

I'm not overly bothered by this, individual crate organization in this sense has to do with how cluttered the crate is. If the crate has a single main type, it's not too bad to clutter the root module. If it has many different types it might want an options module.

Overall options are required to be specified in a lot of places so having short imports is nice. Perhaps we should default to hoisting these types to the top unless the crate has a lot of modules and types, in which case we put them wherever it makes sense.

I think "options types are in a module in some crates and not in others" is low-confusion: I don't think people will be copy pasting code for working with different ICU4X components, they're already quite different.

@robertbastian
Copy link
Member

  • @sffc We have options structs, options enums, and we have the options module. Enums get used as the values in the options structs. I would like the structs and enums to live in the options module. This is not icu_preferences. For preferences, we already decided that they should live in the same module.
  • @zbraniecki My intuitive guess is that the options struct should be re-exported, and everything that it needs is in the options module.
  • @sffc I think we should have the options struct in one place or the other but not both.
  • @zbraniecki The main options struct that is used by the constructors f the main component -- it's nice that you don't have write icu_foo::options::FooOptions, but instead just icu_foo::options::<SomeOption>
  • @sffc A nice thing of having the options module is that it keeps the top-level namespace of the component cleaner. It's fairly discoverable to go to a options module to get it.
  • @robertbastian I think it's fairly standard practice in Rust crates in general to have an options module. I also want to be careful putting things in the top level. If you're looking for a Collator, you want to find Collator, not CollatorOptions and different enums. Saying the one options bag that everything in the crate uses can live at the top level doesn't really scale to ICU4X because we often have multiple options bags in the same crate.
  • @Manishearth My main concern is that I don't like many layers of nesting. More than one is unwieldy to use.
  • @sffc I think we should have exactly one level of nesting.
  • @Manishearth For crates that have one or two options types, I'm okay having them at the top level, but I'm also okay with that preference being overridden by consistency.

Consensus to land a PR putting all options structs and enums in options modules.

@sffc sffc added C-meta Component: Relating to ICU4X as a whole 2.0-breaking Changes that are breaking API changes and removed discuss Discuss at a future ICU4X-SC meeting labels Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0-breaking Changes that are breaking API changes C-meta Component: Relating to ICU4X as a whole
Projects
None yet
Development

No branches or pull requests

3 participants