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

Add JSON data provider with sample number data #61

Merged
merged 37 commits into from
Jun 9, 2020

Conversation

sffc
Copy link
Member

@sffc sffc commented Apr 28, 2020

This PR includes:

  • A DataProvider trait with related structs
  • A JsonDataProvider implementation of the DataProvider trait
  • Test cases demonstrating how it loads number symbols from a JSON data file

@filmil filmil requested review from filmil and zbraniecki May 14, 2020 17:19
@sffc sffc marked this pull request as ready for review May 16, 2020 06:38
@sffc sffc changed the title Add JSON data provider with number data Add JSON data provider with sample number data May 16, 2020
@sffc
Copy link
Member Author

sffc commented May 16, 2020

Okay, I did some cleanup. Let's review this now. This PR contains three main things:

  • A DataProvider trait with related structs
  • A JsonDataProvider implementation of the DataProvider trait
  • Test cases demonstrating how it loads number symbols from a JSON data file

filmil
filmil previously approved these changes Jun 3, 2020
Copy link
Contributor

@filmil filmil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally OK with this commit to be merged. It may not be the last time we touch these files, so even if there are questions still unresolved there's time to address them.

components/datap_json/src/lib.rs Outdated Show resolved Hide resolved
components/util/src/datap/mod.rs Outdated Show resolved Hide resolved
components/util/src/datap/mod.rs Outdated Show resolved Hide resolved
components/util/src/datap/mod.rs Outdated Show resolved Hide resolved
@sffc sffc requested a review from zbraniecki June 4, 2020 17:18
zbraniecki
zbraniecki previously approved these changes Jun 4, 2020
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks generally good. All my comments are nits that we can iron out after landing.

My only high level concern is about component distribution that you're doing.

I'm not sure if separating components/utils from components/datap_json is the right way to go.

My vision would be to have a component called components/data_provider which has optional feature json.
When user includes data_provider as a dependency, they get access to the traits, and when they enable feature json they have access to JSONDataProvider.

@Manishearth - do you know of any prior art in Rust with deciding on such structures?

components/util/src/datap/mod.rs Outdated Show resolved Hide resolved
components/util/src/datap/mod.rs Outdated Show resolved Hide resolved
@sffc
Copy link
Member Author

sffc commented Jun 4, 2020

This looks generally good. All my comments are nits that we can iron out after landing.

My only high level concern is about component distribution that you're doing.

I'm not sure if separating components/utils from components/datap_json is the right way to go.

My vision would be to have a component called components/data_provider which has optional feature json.
When user includes data_provider as a dependency, they get access to the traits, and when they enable feature json they have access to JSONDataProvider.

@Manishearth - do you know of any prior art in Rust with deciding on such structures?

My thinking was that we may have several different data provider definitions, not only things like JSON or CBOR or Bincode, but also ones for caching, delegation, etc. Each type of data provider would go into its own crate. However, I can also see an argument for these to be modules within a single crate.

@zbraniecki
Copy link
Member

However, I can also see an argument for these to be modules within a single crate.

I think that JSON is so omnipresent that it's ok to have it in data_provider itself. If we need CBOR and don't want to have it as a feature in data_provider we can then release data_provider_cbor as a separate crate, but I don't think it makes sense to separate between traits and JSON.

Either way, not blocking. Optional and up to you :)

@Manishearth
Copy link
Member

@zbraniecki I think having a dataprovider crate that is just the traits and then individual crates for e.g. "json data provider" is the right way to go

@sffc sffc dismissed stale reviews from zbraniecki and filmil via 0d6828a June 6, 2020 06:42
@sffc
Copy link
Member Author

sffc commented Jun 6, 2020

OK, I filed follow-up issues for the open questions. I switched from String to LanguageIdentifier (let's discuss whether that's the right choice when we get closer to v1). If all looks good, please give one more rubber stamp so I can check it in.

zbraniecki
zbraniecki previously approved these changes Jun 6, 2020
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agh, another round of nits. Sorry for that. I'm ok with you landing it as is of course, and still am reading and finding those little things :)

Feel free to land and address in a follow up.

Cargo.toml Outdated Show resolved Hide resolved
components/data_provider_json/src/lib.rs Outdated Show resolved Hide resolved
components/data_provider_json/tests/test_file_io.rs Outdated Show resolved Hide resolved
@sffc
Copy link
Member Author

sffc commented Jun 9, 2020

Thanks; I implemented 2 of your comments, and opened #118 to follow up on the third.

Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thank you!

@sffc sffc merged commit 128410e into unicode-org:master Jun 9, 2020
@sffc sffc linked an issue Jun 17, 2020 that may be closed by this pull request
@sffc sffc linked an issue Jun 23, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Where do data struct definitions live? Data provider trait definition
7 participants