Simplify test sources#7906
Conversation
robertbastian
requested review from
a team,
Manishearth and
sffc
as code owners
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
Just looking at hte first commit, I'm not really sure what's going on. The include_files macro seems great (feel free to land separately if you'd like). I don't quite understand the rest of this "cleanup". What is being changed? Why? What's bad with what we have now?
| .unwrap() | ||
| .read_and_parse_toml::<SegmenterRuleTable>(rules_file) | ||
| .expect("The data should be valid!"); | ||
| let segmenter = |
There was a problem hiding this comment.
question: are we not caching anymore? don't we want to cache?
There was a problem hiding this comment.
We don't need caching because these are only loaded once, and even if not, the TOML parsing is only a tiny part of the work that's being done in the implementation.
These don't belong in the icuexportdata source, which I'm trying to slim down.
|
I believe your question refers to the second commit, because I don't see what you wouldn't understand about the others. The second commit changes the download-repo-sources script to use the existing |
|
I mean, I was just reviewing the |
|
simplify just simplifies the generated code, and makes some small changes here and there like introducing some internal constructors which really shouldn't be controversial |
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
Alright, I think I see the pertinent changes.
This
- Moves download-repo-sources into an icu_provider_source "test" so that it can use AbstractFS. I'm not a huge fan of using the test runner for binaries but I guess it works.
- Uses AbstractFS
- Adds a macro that makes filepahts betters
- Stops using AbstractFS for one-off data loads. I'm still not fully sure of this but it's intentional.
Overall seems positive. Want to wait for @sffc's thoughts especially on turning download-repo-sources into a non-test. But I think this is fine.
We already do this for make-testdata. |
|
This is blocking other PRs, can you not just approve? |
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
ok. For future reference, if you want reviews to be faster, please, please provide some context on what you're doing in the PR body. It was not easy to figure out what this PR was trying to do; and why.
|
Hmm, this PR removed |
sffc
left a comment
sffc
left a comment
There was a problem hiding this comment.
I looked at the first commit for a while and am not really sure what it is doing but 🤷.
Moving the tool to be a unit test tool is fine with me. We did it with make-testdata already. I consider it a flaw in Rust that there isn't an easy way to export APIs for local users that aren't included in a crates.io release.
The deleting of the "unused" data files should not have been in this PR. They exist for reasons. Maybe those reasons are flawed, but that should be debated in a standalone PR, not in a "refactor to be merged asap" PR.
3 participants
Split from #7882
Changelog
N/A