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 !

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
provider/datagen/src/source.rs Outdated Show resolved Hide resolved
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)

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.

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

.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.

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 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