add attribute filtering to icu4x-datagen #7236
Conversation
| inverted: bool, | ||
| } | ||
|
|
||
| #[derive(Debug)] |
There was a problem hiding this comment.
Suggestion: use derive(Display) which uses the displaydoc crate to generate a Display impl
| for filter in &cli.attribute_filter { | ||
| let regex = filter.regex.clone(); | ||
| let inverted = filter.inverted; | ||
| driver = driver.with_marker_attributes_filter(&filter.domain, move |attr| { |
There was a problem hiding this comment.
Issue: You should collect the filters for the same domain into a single filter and then call this function. This function drops all previous filters. #7240
| use std::fmt::Display; | ||
| use std::path::PathBuf; | ||
| use std::str::FromStr; | ||
|
|
There was a problem hiding this comment.
Please add a test for this new flag, because it is sufficiently novel that it isn't covered by the ExportDriver tests.
I suggest adding a command to export a human-readable JSON file tree into a tests/filter_test/out directory using an interesting attributes filter.
You can add the command line to generate the JSON here:
Line 33 in f94572c
You can add any additional interesting attributes to the hello world data here. There is already one attribute that is either empty or is the string "reverse":
icu4x/provider/core/src/hello_world.rs
Line 110 in f94572c
2 participants
add a
--attribute-filtercli flag to icu4x-datagen, that implements a regex search like suggested in this review comment chain.as i have implemented something extremely similarly sometime in the past i have some thoughts:
-as suggested in this review, but i wanted to share an alternative: in my previous implementation i used regex "flags" similarly to ecmascript regexes, so i can do a case-insensitive match like/regex/iand an inverted like/regex/v(-vflag from grep) or i can combine them.^(?:)$. this does however have one side effect: the error messages are worse, as the error message now point to the wrapped regex and)x(is now allowed as an input. this is fixable by parsing the regex twice, which i can do if you want me to. however, this is also an issue in ripgrep, so it is probably fine for a cli.resolves #6851
supersedes/closes #7166