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 subset of dates data to DataProvider #256

Merged
merged 7 commits into from
Sep 22, 2020

Conversation

zbraniecki
Copy link
Member

This PR introduces a subset of ca-gregorian from cldr-dates that will be needed for initial DateTimeFormat.

See: https://github.com/unicode-cldr/cldr-dates-modern/blob/master/main/en/ca-gregorian.json

The PR operates on Cow everywhere, but once we land DateTime we may want to switch some of them to Pattern structs if we measure performance wins with no significant payload growth.

@zbraniecki zbraniecki self-assigned this Sep 20, 2020
@zbraniecki zbraniecki added this to the ICU4X 0.1 milestone Sep 20, 2020
@coveralls
Copy link

coveralls commented Sep 20, 2020

Pull Request Test Coverage Report for Build d5278718a395b54b2e37b937155eae457a452eb4-PR-256

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+4.005%) to 82.941%

Files with Coverage Reduction New Missed Lines %
components/data-provider/src/data_key.rs 2 25.0%
Totals Coverage Status
Change from base Build 1f08acccfec3fd1800c8fad8fbe43fabb3ffb652: 4.005%
Covered Lines: 1867
Relevant Lines: 2251

💛 - Coveralls

@zbraniecki zbraniecki mentioned this pull request Sep 20, 2020
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Thanks! This is definitely on the right track. Left some comments.

components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Show resolved Hide resolved
@zbraniecki
Copy link
Member Author

Thanks for the review! I filed #257 to discuss how should request/response look like for Dates.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/data-provider/src/structs/dates.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/data-provider/src/structs/dates.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/data-provider/src/structs/dates.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@zbraniecki
Copy link
Member Author

I updated the PR to reflect Shane's initial comments.

I have two follow up PRs that I'd like to start getting ready to land, but cycling 4 PRs in git is a mess (Mercurial solved it years ago so well!).

Shane, how far do you think this is from being landable?
I understand that we'll continue iterating over it as I pull DateTimeFormat and this together, and I'm comfortable removing DateTimeFormat from 0.1 release if we feel like it's not in shape by the time 0.1 is ready, but I'm concerned about the mental overhead of juggling those PRs.

@sffc
Copy link
Member

sffc commented Sep 20, 2020

You are using Options in order to make data requests smaller. But, at the same time, you are putting everything in one big struct. I think that's a conflicted design that we need to solve.

It makes no sense to me to have Options for symbols and date patterns. What are you supposed to do if the data provider gives you one but not the other? Option should be used only if you can fall back within the data key, such as between plural forms, where Option is warranted. Fallback between locales should be handled on the data provider level. ICU4X should never need to request the same key multiple times from the data provider!

I would like to either:

  1. Remove the Options, or
  2. Split the data into the three keys I suggested (datesym, timesym, and dtpatterns).

@zbraniecki
Copy link
Member Author

@sffc I removed the options, and we can revisit the splits later.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/data-provider/src/structs/dates.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

sffc
sffc previously approved these changes Sep 21, 2020
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Thanks! I'm approving but I still have questions/comments/suggestions.

components/data-provider/src/structs/dates.rs Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Show resolved Hide resolved
components/data-provider/src/structs/dates.rs Outdated Show resolved Hide resolved
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/data-provider/src/structs/dates.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/data-provider/src/structs/dates.rs is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

sffc
sffc previously approved these changes Sep 22, 2020
sffc
sffc previously approved these changes Sep 22, 2020
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

LGTM. We should have a real Default implementation for the date symbols, but we can do that later:

#260

@zbraniecki zbraniecki merged commit ab00f54 into unicode-org:master Sep 22, 2020
zbraniecki added a commit that referenced this pull request Sep 23, 2020
* Add support for Dates to CLDRJsonDataProvider

* Apply feedback
@zbraniecki zbraniecki deleted the dates-provider branch October 19, 2020 15:49
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.

3 participants