-
Notifications
You must be signed in to change notification settings - Fork 234
Add --attribute-allowlist and --attribute-denylist options for icu4x-… #7166
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
base: main
Are you sure you want to change the base?
Conversation
9ba010a to
8b68b08
Compare
8b68b08 to
89954bb
Compare
|
|
||
| #[arg(long, num_args = 2.., value_names = ["DOMAIN", "ATTRIBUTES"])] | ||
| #[arg( | ||
| help = "Filter attributes on markers for a domain. Accepts two or more arguments.\n\ |
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.
suggestion: instead of accepting a list of exact matches, it's probably more user-friendly (and more expressiv) to accept a regex. we already have the regex crate in the transitive dependencies anyway
we'd also not need both an allowlist and a denylist
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'd also not need both an allowlist and a denylist
I'm not sure about this: inverting regexes is really annoying. having a way to both allowlist and denylist by-regex would be good, as long as we ensure they never clash
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 could have some special negate syntax as part of the values as well. In any case we need to figure out how an overlap of allow and deny should be handled when there are multiple filters for the same domain.
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.
That would be fine.
As for overlaps: I was thinking that we simply disallow them; in theory we could pick a priority and support it (or have it based on declaration order), but 99% of use cases will be covered by a single filter.
Having a single attribute with +/- syntax seems great.
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.
It's really nontrivial to design a good +/- syntax. It would be nice to avoid it if we have a better alternative. I've found from experience that the all/any/not thing used in cfg is the simplest design that covers basically all the use cases.
I wouldn't mind something like:
# Include one numbering system, latn
--marker-attributes-filter numbering_system=latn
--marker-attributes-filter numbering_system=/^latn$/
# Include latn and thai numbering systems
--marker-attribute-filter numbering_system=any(latn,thai)
--marker-attribute-filter numbering_system=/^(latn|thai)$/
# Include all numbering systems except for thai
--marker-attribute-filter numbering_system=not(thai)
--marker-attribute-filter numbering_system=/^(?!thai$).*/
I think string literals with all/any/not would cover 90% of use cases.
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 not excited about an allowlist/denylist filter argument pair. I think a single filter is a better path forward and we can iterate on the DSL in that context.
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 mostly worried about designing ourselves out of a convenient DSL by implementing it partially. Especially since parsing regexes is nontrivial.
Personally I weakly consider negative filtering to be a part of the MVP. I don't actually think there are other features we need in the DSL.
I think --filter="attr1=/foo/" is probably okay since it's easy to add attr1=-/foo/ later. As long as our DSL supports at most one filter per argument we're fine, we shouldn't be in a situation where we have to parse --filters="attr1=filter1 attr2=filter2" because then you need to learn where regexes end.
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 think the simplest and most foolproof implementation might be to allow negating the whole regex, and if the argument is specified multiple times, then perform the checks in series.
Example Args:
--marker-attributes-filter domain1=/^[abc].*$/
--marker-attributes-filter domain1=!/^a[de].*$/
--marker-attributes-filter domain1=/^aef.*$/
--marker-attributes-filter domain2=/^(abc|xyz)$/
This gets compiled into an ordered list of rules for domain1 and an ordered list of rules for domain2. If the first rule is positive, then the initial set is empty. If the first rule is negative, then the initial set is the whole set.
As an onramp, we can accept either a single positive regex or a single negative regex, and reject if the same domain was provided multiple times.
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.
To make it easier to type in Bash, probably omit the ^ and $ in the regex, and use - instead of ! for negation.
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 think that's a good starting point. I am worried about ^ and $ though, but I think if we always implicitly check for whole-string match users can always use * to do partial matches.
|
|
||
| #[arg(long, num_args = 2.., value_names = ["DOMAIN", "ATTRIBUTES"])] | ||
| #[arg( | ||
| help = "Filter attributes on markers for a domain. Accepts two or more arguments.\n\ |
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: it should also be possible to add two filters for different domains
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.
Agreed.
Implemented #6851