Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

noisysocks
Copy link
Contributor

Expands on #613 and adds two scripts:

  1. one that generates a JSON schema file, which we commit to the repo, and;
  2. one that validates our rules against the JSON schema.

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:

Kapture 2025-01-30 at 11 50 18

https://json-schema.org/

@noisysocks noisysocks mentioned this pull request Jan 30, 2025
@noisysocks noisysocks requested a review from muodov January 30, 2025 01:01
Copy link
Member

@muodov muodov left a 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
Comment on lines 23 to 24
"generate-rule-schema": "ts-node scripts/generate-rule-schema.ts",
"validate-rules": "ts-node scripts/validate-rules.ts",
Copy link
Member

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.

Copy link
Contributor Author

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.

package.json Show resolved Hide resolved
@@ -1,4 +1,5 @@
{
"$schema": "https://raw.githubusercontent.com/duckduckgo/autoconsent/refs/heads/main/rules/schema.json",
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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 $schemas 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.

@muodov muodov requested a review from sammacbeth January 30, 2025 15:36
Base automatically changed from validate-json-rules to main January 31, 2025 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants