-
Notifications
You must be signed in to change notification settings - Fork 22
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
Split rule-syntax-check into generate-rule-schema and validate-rules #615
base: main
Are you sure you want to change the base?
Conversation
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.
Looks nice overall, just a few maintainability concerns
package.json
Outdated
"generate-rule-schema": "ts-node scripts/generate-rule-schema.ts", | ||
"validate-rules": "ts-node scripts/validate-rules.ts", |
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.
ts-node
is a new dependency that we'd need to consider carefully. For now, sticking with plain JS for CLI scripts and using JSDoc comments for typing would be more consistent with other DDG repos.
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.
Is there a particular concern you have with ts-node
? get-text-for-xpath
(#386) is an existing script that uses it. I think TypeScript is nice for scripts as it gives you a little bit of extra confidence in an environment that doesn’t usually have unit tests. But it's not a strong opinion, happy to defer to you on this.
@@ -1,4 +1,5 @@ | |||
{ | |||
"$schema": "https://raw.githubusercontent.com/duckduckgo/autoconsent/refs/heads/main/rules/schema.json", |
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.
is it possible to point to a local 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.
Seems to work fine in my testing. It might be IDE dependent. We can do local refs for now until it’s a problem or we have a need for third party rules. 6cc349d
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.
Actually, hm, I’m having second thoughts. Using ”../schema.json”
as the $schema
means that the combined rules in rules/rules.json
have invalid $schema
s as they’re in a different directory. It seems likely to me that these rules will move around as the project evolves and we explore remote configuration etc. Using an absolute URL ensures that rules are atomic: you can move them, copy/paste them, whatever, and they remain valid.
I reverted 6cc349d.
6cc349d
to
0ea8913
Compare
This reverts commit 0ea8913.
Expands on #613 and adds two scripts:
This way we can reference the committed JSON schema using
$schema
at the top of each rule and developers get autocompletion and hints in their IDE when writing and editing rules:https://json-schema.org/