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

Moving source data out of icu_testdata #3044

Merged
merged 32 commits into from
Feb 21, 2023

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Jan 30, 2023

  • Moves provider/testdata/data/{cldr,icuexport} to provider/repodata/data
  • Shifts the source of truth for testdata metadata into
    • tools/testdata-scripts/locales.rs.data
    • SourceData::{LATEST_TESTED_CLDR_TAG, LATEST_TESTED_ICUEXPORT_TAG}
      • These were duplicated in testdata metadata before
  • make-testdata generates provider/testdata/data/metadata.rs.data from these
  • This removes the testdata-scripts -> icu_testdata dependency, and fixes cargo quick yields compile errors #3013

Depends on #3042
Part of #3038

@jira-pull-request-webhook

This comment was marked as spam.

@robertbastian robertbastian requested review from sffc and removed request for zbraniecki and nciric January 30, 2023 22:44
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.

Will review again after the dependent PRs are resolved

@dpulls
Copy link

dpulls bot commented Feb 1, 2023

🎉 All dependencies have been resolved !

@robertbastian robertbastian requested a review from sffc February 1, 2023 20:16
components/locid/src/databake.rs Show resolved Hide resolved
provider/testdata/src/lib.rs Show resolved Hide resolved
provider/datagen/src/source.rs Outdated Show resolved Hide resolved
@robertbastian robertbastian requested a review from sffc February 1, 2023 22:24
provider/datagen/src/source.rs Outdated Show resolved Hide resolved
@robertbastian robertbastian requested a review from sffc February 3, 2023 16:23
@robertbastian robertbastian requested a review from sffc February 13, 2023 13:21
@robertbastian robertbastian requested a review from sffc February 14, 2023 09:59
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.

(replied to open issue; still unresolved feedback)

@robertbastian robertbastian requested a review from sffc February 14, 2023 22:44
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.

We're still not aligned on the new location for the source data cache.

@robertbastian robertbastian requested a review from sffc February 17, 2023 23:28
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.

I'm happy with this now. Just one nit. Please also make CI happy.

@@ -0,0 +1,27 @@
// This file is part of ICU4X. For terms of use, please see the file
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please update .gitattributes with the new paths for generated data files

@robertbastian robertbastian requested a review from sffc February 21, 2023 19:31
.gitattributes Outdated
provider/testdata/data/json/** linguist-generated=true
provider/testdata/data/cldr/** linguist-generated=true
provider/testdata/data/icuexport/** linguist-generated=true
provider/testdata/data/** linguist-generated=true
Copy link
Member

Choose a reason for hiding this comment

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

Issue: We had these pointing at subdirectories because there are certain files we don't want to hide away, like the fingerprint files

Copy link
Member Author

Choose a reason for hiding this comment

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

They are generated though

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but the reason we use this feature is to control GitHub reviews, and it's important that fingerprint files are part of reviews. We had this discussion when we first added the linguist-generated attributes.

@robertbastian robertbastian requested a review from sffc February 21, 2023 21:21
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.

Pong

@robertbastian robertbastian requested a review from sffc February 21, 2023 22:59
@robertbastian robertbastian merged commit 1688d9f into unicode-org:main Feb 21, 2023
@robertbastian robertbastian deleted the datastuff branch February 22, 2023 20:24
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.

cargo quick yields compile errors
2 participants