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

Reconsider location for data struct definitions #415

Closed
sffc opened this issue Dec 9, 2020 · 6 comments · Fixed by #451
Closed

Reconsider location for data struct definitions #415

sffc opened this issue Dec 9, 2020 · 6 comments · Fixed by #451
Assignees
Labels
A-design Area: Architecture or design C-data-infra Component: provider, datagen, fallback, adapters T-core Type: Required functionality
Milestone

Comments

@sffc
Copy link
Member

sffc commented Dec 9, 2020

With #405, it will no longer be necessary to keep all of the data struct definitions in a common crate. Previously, it was required that all data providers have access to the struct definitions when deserializing data. However, with this no longer the case, we should reconsider this design choice.

@sffc sffc added C-data-infra Component: provider, datagen, fallback, adapters A-design Area: Architecture or design discuss Discuss at a future ICU4X-SC meeting labels Dec 9, 2020
@sffc sffc self-assigned this Dec 9, 2020
@sffc sffc removed the discuss Discuss at a future ICU4X-SC meeting label Dec 10, 2020
@sffc sffc added this to the 2020 Q4 milestone Dec 10, 2020
@sffc sffc added the T-core Type: Required functionality label Dec 10, 2020
@sffc
Copy link
Member Author

sffc commented Dec 17, 2020

Last week, we discussed this and decided it is a good idea to distribute the data structs into their individual crates.

One catch I discovered: the InvariantDataProvider should be able to construct instances of all of these structs. Options:

  1. Make a separate crate InvariantDataProvider and have it depend on all crates with data struct definitions
  2. Allow crates to register structs to a global registry for the purposes of InvariantDataProvider
  3. Keep the struct definitions in a common crate

I like option 2, because it is the most generalizable, such as people adding new struct definitions outside of code ICU4X.

@sffc sffc added the discuss Discuss at a future ICU4X-SC meeting label Dec 17, 2020
@zbraniecki
Copy link
Member

I'm strongly in favor of 2

@sffc
Copy link
Member Author

sffc commented Dec 17, 2020

OK. I'm not sure where to start on implementing option 2. Does Rust have static initializers or some other way to register functions to be run at startup? Or some way to globally loop through all implementations of a certain trait?

@zbraniecki
Copy link
Member

We could use lazy_static or phf - http://siciarz.net/24-days-rust-static-initialization/

@sffc
Copy link
Member Author

sffc commented Dec 17, 2020

2020-12-17: Global registries are just bad; probably don't use one. Consider other options, like generic functions powering the InvariantDataProvider. @sffc to read the blog post linked above.

@sffc
Copy link
Member Author

sffc commented Dec 30, 2020

When we move the struct definitions, we can also make serde an optional feature on the icu_provider crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-design Area: Architecture or design C-data-infra Component: provider, datagen, fallback, adapters T-core Type: Required functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants