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 2 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 Show resolved Hide resolved
package.json Outdated 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
@noisysocks noisysocks force-pushed the rules-json-schema branch 2 times, most recently from f5805e8 to 70ba048 Compare February 5, 2025 01:34
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this use .mjs / ESM to match the other scripts in scripts?

@noisysocks
Copy link
Contributor Author

noisysocks commented Feb 5, 2025

@muodov @sammacbeth: Rebased this on main. What do you think? I’d love to have it as a QoL improvement. I keep making silly mistakes (e.g. passing strings where arrays are expected, arrays where strings are expected) while writing new rules 😀

@noisysocks noisysocks requested a review from muodov February 5, 2025 01:37
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