-
Notifications
You must be signed in to change notification settings - Fork 13.4k
jsondocck: Refactor directive handling #141709
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: master
Are you sure you want to change the base?
Conversation
/// `//@ !has <path>` | ||
/// | ||
/// Checks the path doesn't exist. | ||
HasNotPath, |
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 sounds a bit weird to me, needed to read the docs (yeay docs!) to get what it was about. Also, is it really worth it to have two variants for each command and its opposite? Why not having a not: bool
argument (or field directly in the enum if you prefer?) instead?
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.
_ => panic!("`//@ !has` must have 2 or 3 arguments, but got {args:?}"), | ||
}, | ||
|
||
(_, false) if KNOWN_DIRECTIVE_NAMES.contains(&directive_name) => { |
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.
Shouldn't it fail here saying it doesn't support the not
mode for this command?
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.
What do you mean? This is for things that arn't negeated (e.g. //@ only-aarch64
), which we should silently ignore. For //@ !only-aarch64
, then negated
will be true, this case won't match, and we'll panic the the wildcard below.
7f54d30
to
508479d
Compare
508479d
to
14db1b5
Compare
Best reviewed commit by commit.
r? @GuillaumeGomez