Repository: stoplightio/spectral
Pull request #1094 (2020-04-15 09:38:37)

fix: ensure all rules are recommended by default
As of Spectral 5, all rules are recommended by default now. However, no normalization is applied to the `recommended` property (as it's done for `severity`, for instance). This PR fixes this.

Also slightly enhance the code coverage to ensure the `recommended` setting of a rule eventually leads to the rule being run or not.

**Checklist**

- [x] Tests added / updated
- [ ] Docs added / updated

**Does this PR introduce a breaking change?**

- [ ] Yes
- [x] No


nulltoken: > Hm, I might be missing something, but what does this PR fix actually?
> All rules are recommended by default unless you set recommended to false.

@P0lip The first commit that triggered that PR was to actually drop the unneeded `recommended: true` settings in the oas ruleset.

And... tests started to fail.

So I took a look at the normalization process and realized that although we always make `severity` explicit (even when `Warning` is the default), we don't do the same for `recommended`.

This PR attempts at aligning this so that rulesets can be designed with implicit settings and rules, when materialized, always expose explicit settings.

Thoughts?
nulltoken: > > Hm, I might be missing something, but what does this PR fix actually?
> > All rules are recommended by default unless you set recommended to false.
> 
> @P0lip The first commit that triggered that PR was to actually drop the unneeded `recommended: true` settings in the oas ruleset.
> 
> And... tests started to fail.
> 
> So I took a look at the normalization process and realized that although we always make `severity` explicit (even when `Warning` is the default), we don't do the same for `recommended`.
> 
> This PR attempts at aligning this so that rulesets can be designed with implicit settings and rules, when materialized, always expose explicit settings.
> 
> Thoughts?

@P0lip If you don't think making `recommended` value explicit is worth it, I could also tune down that PR to only focus on dropping the `recommended: true` settings from the oas ruleset and fixing the tests.
P0lip: > @P0lip The first commit that triggered that PR was to actually drop the unneeded recommended: true settings in the oas ruleset.

Ah, so I see, but in any case I don't think we should drop them - they make everything more readable IMHO. Otherwise a user need to go to docs to see how recommended works.

> And... tests started to fail.

They might have started to fail because we don't normalize the rules, but we still should treat them in the same way, meaning they are not run or run depending on the ruleset you use.

> @P0lip If you don't think making recommended value explicit is worth it, I could also tune down that PR to only focus on dropping the recommended: true settings from the oas ruleset and fixing the tests.

Yeah, I think I'd leave it. IMHO it's just more readable, but it might be just me.


That being said, I'm still open for having a test that actually verifies whether we run a given rule or not. We might have such, but maybe not, so if you could verify that and write one in case we don't, I'd be very grateful
nulltoken: @P0lip Thanks for the feedback!

So, just to make sure I got your right:
- ok to explicitly normalize the `recommended` setting to `true` while reading the ruleset
- keep the `recommended: true` settings in the oas ruleset
- ensure `recommended` rules are run and not `!recommended` aren't (either find a test that does so or write one)

Would that fit you?
P0lip: @nulltoken yeah, that works!
nulltoken: @P0lip I believe I've dealt with all your comments, Sir!

Patch:
diff --git a/src/rulesets/mergers/rules.ts b/src/rulesets/mergers/rules.ts
index 3e4f152c..d86685f0 100644
--- a/src/rulesets/mergers/rules.ts
+++ b/src/rulesets/mergers/rules.ts
@@ -117,6 +117,10 @@ function processRule(rules: FileRuleCollection, name: string, rule: FileRule | F
 }
 
 function normalizeRule(rule: Rule, severity: DiagnosticSeverity | HumanReadableDiagnosticSeverity | undefined) {
+  if (rule.recommended === void 0) {
+    rule.recommended = true;
+  }
+
   if (rule.severity === void 0) {
     rule.severity = severity === void 0 ? (rule.recommended !== false ? DEFAULT_SEVERITY_LEVEL : -1) : severity;
   } else {


Test patch:
diff --git a/src/__tests__/__fixtures__/recommended-ruleset.json b/src/__tests__/__fixtures__/recommended-ruleset.json
new file mode 100644
index 00000000..79e1b96a
--- /dev/null
+++ b/src/__tests__/__fixtures__/recommended-ruleset.json
@@ -0,0 +1,33 @@
+{
+  "rules": {
+    "explicitly-recommended": {
+      "message": "Should expose a `likeable` property",
+      "given": "$",
+      "type": "style",
+      "recommended": true,
+      "then": {
+        "field": "likeable",
+        "function": "truthy"
+      }
+    },
+    "implicitly-recommended": {
+      "message": "Should expose a `likeable` property",
+      "given": "$",
+      "type": "style",
+      "then": {
+        "field": "likeable",
+        "function": "truthy"
+      }
+    },
+    "explicitly-not-recommended": {
+      "message": "Should expose a `likeable` property",
+      "given": "$",
+      "type": "style",
+      "recommended": false,
+      "then": {
+        "field": "likeable",
+        "function": "truthy"
+      }
+    }
+  }
+}
diff --git a/src/__tests__/linter.jest.test.ts b/src/__tests__/linter.jest.test.ts
index db61f879..3be14228 100644
--- a/src/__tests__/linter.jest.test.ts
+++ b/src/__tests__/linter.jest.test.ts
@@ -16,6 +16,7 @@ import { IRuleset, RulesetExceptionCollection } from '../types/ruleset';
 const customFunctionOASRuleset = path.join(__dirname, './__fixtures__/custom-functions-oas-ruleset.json');
 const customOASRuleset = path.join(__dirname, './__fixtures__/custom-oas-ruleset.json');
 const customDirectoryFunctionsRuleset = path.join(__dirname, './__fixtures__/custom-directory-function-ruleset.json');
+const recommendedRulesetPath = path.join(__dirname, './__fixtures__/recommended-ruleset.json');
 
 describe('Linter', () => {
   let spectral: Spectral;
@@ -724,4 +725,31 @@ console.log(this.cache.get('test') || this.cache.set('test', []).get('test'));
       });
     });
   });
+
+  test('should only run recommended rules, whether implicitly or explictly', async () => {
+    const target = {
+      openapi: '3.0.2',
+    };
+
+    await spectral.loadRuleset(recommendedRulesetPath);
+
+    expect(Object.keys(spectral.rules)).toHaveLength(3);
+
+    expect(Object.entries(spectral.rules).map(([name, rule]) => [name, rule.recommended])).toEqual([
+      ['explicitly-recommended', true],
+      ['implicitly-recommended', true],
+      ['explicitly-not-recommended', false],
+    ]);
+
+    const res = await spectral.run(target);
+
+    expect(res).toEqual([
+      expect.objectContaining({
+        code: 'explicitly-recommended',
+      }),
+      expect.objectContaining({
+        code: 'implicitly-recommended',
+      }),
+    ]);
+  });
 });
diff --git a/src/__tests__/spectral.jest.test.ts b/src/__tests__/spectral.jest.test.ts
index 91250d53..6cb2ab2d 100644
--- a/src/__tests__/spectral.jest.test.ts
+++ b/src/__tests__/spectral.jest.test.ts
@@ -32,6 +32,7 @@ describe('Spectral', () => {
               name,
               ...rule,
               formats: expect.arrayContaining([expect.any(String)]),
+              recommended: expect.any(Boolean),
               severity: expect.any(Number),
               then: expect.any(Object),
             };
@@ -86,6 +87,7 @@ describe('Spectral', () => {
         'info-matches-stoplight': {
           ...ruleset.rules['info-matches-stoplight'],
           name: 'info-matches-stoplight',
+          recommended: true,
           severity: DiagnosticSeverity.Warning,
         },
       });
diff --git a/src/__tests__/spectral.karma.test.ts b/src/__tests__/spectral.karma.test.ts
index 94fcc7b1..b446f714 100644
--- a/src/__tests__/spectral.karma.test.ts
+++ b/src/__tests__/spectral.karma.test.ts
@@ -45,6 +45,7 @@ describe('Spectral', () => {
         'info-matches-stoplight': {
           ...ruleset.rules['info-matches-stoplight'],
           name: 'info-matches-stoplight',
+          recommended: true,
           severity: DiagnosticSeverity.Warning,
         },
       });
diff --git a/src/__tests__/spectral.test.ts b/src/__tests__/spectral.test.ts
index c8c851c1..f5e5e1a0 100644
--- a/src/__tests__/spectral.test.ts
+++ b/src/__tests__/spectral.test.ts
@@ -27,6 +27,7 @@ describe('spectral', () => {
               name,
               ...rule,
               formats: expect.arrayContaining([expect.any(String)]),
+              recommended: expect.any(Boolean),
               severity: expect.any(Number),
               then: expect.any(Object),
             };
@@ -47,6 +48,7 @@ describe('spectral', () => {
             name,
             ...rule,
             formats: expect.arrayContaining([expect.any(String)]),
+            recommended: expect.any(Boolean),
             severity: expect.any(Number),
             then: expect.any(Object),
           };
diff --git a/src/rulesets/__tests__/reader.jest.test.ts b/src/rulesets/__tests__/reader.jest.test.ts
index 0a860a16..47d30309 100644
--- a/src/rulesets/__tests__/reader.jest.test.ts
+++ b/src/rulesets/__tests__/reader.jest.test.ts
@@ -56,6 +56,7 @@ describe('Rulesets reader', () => {
             given: '$.info',
             message: 'should be OK',
             severity: DiagnosticSeverity.Warning,
+            recommended: true,
             then: expect.any(Object),
           },
           'valid-rule-recommended': {
@@ -78,6 +79,7 @@ describe('Rulesets reader', () => {
             given: '$.info',
             message: 'should be OK',
             severity: DiagnosticSeverity.Warning,
+            recommended: true,
             then: expect.any(Object),
           },
           'valid-rule-recommended': {
@@ -91,6 +93,7 @@ describe('Rulesets reader', () => {
             given: '$.info',
             message: 'should be OK',
             severity: DiagnosticSeverity.Warning,
+            recommended: true,
             then: expect.any(Object),
           },
         },
@@ -114,6 +117,7 @@ describe('Rulesets reader', () => {
       'foo-rule': {
         given: '$.info',
         message: 'should be OK',
+        recommended: true,
         severity: DiagnosticSeverity.Warning,
         then: {
           function: expect.stringMatching(/^random-id-\d$/),
@@ -142,6 +146,7 @@ describe('Rulesets reader', () => {
             ...rule,
             formats: expect.arrayContaining([expect.any(String)]),
             ...((rule as IRule).severity === void 0 && { severity: DiagnosticSeverity.Warning }),
+            ...((rule as IRule).recommended === void 0 && { recommended: true }),
             then: expect.any(Object),
           };
 
@@ -151,6 +156,7 @@ describe('Rulesets reader', () => {
         'valid-rule': {
           given: '$.info',
           message: 'should be OK',
+          recommended: true,
           severity: DiagnosticSeverity.Warning,
           then: expect.any(Object),
         },
@@ -168,6 +174,7 @@ describe('Rulesets reader', () => {
               formats: expect.arrayContaining([expect.any(String)]),
               ...((rule as IRule).severity === undefined && { severity: DiagnosticSeverity.Warning }),
               ...((rule as IRule).recommended === false && { severity: -1 }),
+              ...((rule as IRule).recommended === void 0 && { recommended: true }),
               then: expect.any(Object),
             };
 
@@ -177,6 +184,7 @@ describe('Rulesets reader', () => {
           'valid-rule': {
             given: '$.info',
             message: 'should be OK',
+            recommended: true,
             severity: DiagnosticSeverity.Warning,
             then: expect.any(Object),
           },
@@ -207,6 +215,7 @@ describe('Rulesets reader', () => {
               formats: expect.arrayContaining([expect.any(String)]),
               ...((rule as IRule).severity === void 0 && { severity: DiagnosticSeverity.Warning }),
               ...((rule as IRule).recommended === false && { severity: -1 }),
+              ...((rule as IRule).recommended === void 0 && { recommended: true }),
               then: expect.any(Object),
             };
 
@@ -398,6 +407,7 @@ describe('Rulesets reader', () => {
           given: '$.info',
           message: 'Title must contain Stoplight',
           severity: DiagnosticSeverity.Warning,
+          recommended: true,
           then: {
             field: 'title',
             function: 'pattern',
@@ -449,6 +459,7 @@ describe('Rulesets reader', () => {
         'title-matches-stoplight': {
           given: '$.info',
           message: 'Title must contain Stoplight',
+          recommended: true,
           severity: DiagnosticSeverity.Warning,
           then: {
             field: 'title',
@@ -502,6 +513,7 @@ describe('Rulesets reader', () => {
           given: '$.info',
           message: 'Title must contain Stoplight',
           severity: -1,
+          recommended: true,
           then: {
             field: 'title',
             function: 'pattern',
@@ -522,6 +534,7 @@ describe('Rulesets reader', () => {
           PascalCase: {
             given: '$',
             message: 'bar',
+            recommended: true,
             severity: DiagnosticSeverity.Warning,
             then: {
               function: 'truthy',
@@ -539,6 +552,7 @@ describe('Rulesets reader', () => {
           snake_case: {
             given: '$',
             message: 'foo',
+            recommended: true,
             severity: DiagnosticSeverity.Warning,
             then: {
               function: 'truthy',
@@ -580,6 +594,7 @@ describe('Rulesets reader', () => {
         'valid-foo-value': {
           given: '$',
           severity: DiagnosticSeverity.Warning,
+          recommended: true,
           then: {
             field: 'foo',
             function: 'random-id-0',
@@ -638,6 +653,7 @@ describe('Rulesets reader', () => {
         'valid-foo-value': {
           given: '$',
           severity: DiagnosticSeverity.Warning,
+          recommended: true,
           then: {
             field: 'foo',
             function: 'random-id-0',
@@ -779,6 +795,7 @@ describe('Rulesets reader', () => {
         'bar-rule': {
           given: '$.bar',
           message: 'Bar is truthy',
+          recommended: true,
           severity: DiagnosticSeverity.Warning,
           then: {
             function: 'truthy',
@@ -787,6 +804,7 @@ describe('Rulesets reader', () => {
         'foo-rule': {
           given: '$.foo',
           message: 'Foo is falsy',
+          recommended: true,
           severity: DiagnosticSeverity.Warning,
           then: {
             function: 'falsy',
diff --git a/src/rulesets/mergers/__tests__/rules.test.ts b/src/rulesets/mergers/__tests__/rules.test.ts
index f3009809..3caf55a9 100644
--- a/src/rulesets/mergers/__tests__/rules.test.ts
+++ b/src/rulesets/mergers/__tests__/rules.test.ts
@@ -1,4 +1,4 @@
-import { DiagnosticSeverity } from '@stoplight/types';
+import { DiagnosticSeverity, Dictionary } from '@stoplight/types';
 import { IRule } from '../../../types';
 import { FileRuleCollection } from '../../../types/ruleset';
 import { mergeRules } from '../rules';
@@ -190,7 +190,7 @@ describe('Ruleset rules merging', () => {
   });
 
   it('picks up recommended rules', () => {
-    const rules = {};
+    const rules: Dictionary<IRule, string> = {};
 
     const test3 = JSON.parse(JSON.stringify(baseRule));
     delete test3.recommended;
@@ -208,6 +208,10 @@ describe('Ruleset rules merging', () => {
       'recommended',
     );
 
+    expect(rules.test.recommended).toBe(true);
+    expect(rules.test2.recommended).toBe(false);
+    expect(rules.test3.recommended).toBe(true);
+
     expect(rules).toHaveProperty('test.severity', DiagnosticSeverity.Warning);
     expect(rules).toHaveProperty('test2.severity', -1);
     expect(rules).toHaveProperty('test3.severity', DiagnosticSeverity.Warning);