feat(valid-scripts): add valid-scripts rule (#839) #903
Conversation
This change adds a new `valid-scripts` rule, which is also included in the `recommended` config.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #903 +/- ##
==========================================
+ Coverage 98.77% 98.83% +0.06%
==========================================
Files 21 22 +1
Lines 1222 1287 +65
Branches 142 151 +9
==========================================
+ Hits 1207 1272 +65
Misses 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| emptyScript: | ||
| 'values contained in "scripts" object should not be empty', | ||
| emptyScriptsField: '"scripts" field should not be empty.', | ||
| incorrectTypeScript: |
There was a problem hiding this comment.
[Praise] Haha, "TypeScript". I thought this was cute. 🙂
| messageId: "incorrectTypeScript", | ||
| node: node.value as unknown as ESTree.Node, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
[Question] Why return;? If multiple scripts are incorrect, we'd want to get reports on all of them I think.
E.g. here I'd expect the values of both script1 and script3 to have complaints:
"scripts": {
"script1": "",
"script2": "...",
"script3": "",
}(here and the other return)
| { | ||
| code: `{ "scripts": { "test": undefined }}`, | ||
| errors: [{ messageId: "incorrectTypeScript" }], | ||
| }, |
There was a problem hiding this comment.
[Testing] No need, undefined isn't valid JSON.
| { | |
| code: `{ "scripts": { "test": undefined }}`, | |
| errors: [{ messageId: "incorrectTypeScript" }], | |
| }, |
I'm actually surprised this doesn't cause any "hey you have invalid stuff" complaint in the tester...
| errors: [{ messageId: "incorrectTypeScript" }], | ||
| }, | ||
| ], | ||
| valid: [ |
There was a problem hiding this comment.
[Testing] Some more valid cases to think of:
- No
"scripts"key at all - Empty
"scripts" - Just one value in
"scripts"
| import { ruleTester } from "./ruleTester.js"; | ||
|
|
||
| ruleTester.run("valid-scripts", rule, { | ||
| invalid: [ |
There was a problem hiding this comment.
[Testing] Some more invalid cases to think of:
- 1 incorrect value + 2 correct values
- 2 incorrect values
- duplicate values
- 2 incorrect values + 1 correct value
There was a problem hiding this comment.
[Feature] Fun fact, it is technically valid JSON to have duplicate keys:
"scripts": {
"this-is": "fine",
"this-is": "fine"
}cc @michaelfaith to check me here, but IMO this should be reported on by valid-scripts.
|
|
||
| <!-- end auto-generated rule header --> | ||
|
|
||
| This rule applies two validations to each property in `"scripts"` field: |
There was a problem hiding this comment.
[Docs] Just nitpicking a little bit for clarity, WDYT?
| This rule applies two validations to each property in `"scripts"` field: | |
| This rule checks that if a `"scripts"` property exists, it is a non-empty object. | |
| Additionally, for each of its script properties: |
JoshuaKGoldberg
added
the
status: waiting for author
This change adds a new
valid-scriptsrule, which is also included in therecommendedconfig.PR Checklist
status: accepting prsOverview
This rule checks for
"scripts"field to be an non-empty object.This rule checks that each property in
"scripts"field:🖤