-
Notifications
You must be signed in to change notification settings - Fork 184
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
Conversation
35f0642
to
f07993a
Compare
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this 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
🎉 All dependencies have been resolved ! |
There was a problem hiding this 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)
There was a problem hiding this 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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are generated though
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pong
provider/testdata/data/{cldr,icuexport}
toprovider/repodata/data
tools/testdata-scripts/locales.rs.data
SourceData::{LATEST_TESTED_CLDR_TAG, LATEST_TESTED_ICUEXPORT_TAG}
make-testdata
generatesprovider/testdata/data/metadata.rs.data
from thesetestdata-scripts
->icu_testdata
dependency, and fixescargo quick
yields compile errors #3013Depends on #3042
Part of #3038