feat: drop oas2 and oas3 rulesets #773

nulltoken · 2019-11-16T15:03:15Z

#689 Has been superseded by #772
This PR superseds #772

This is an attempt at helping get all the tests pass.

nulltoken · 2019-11-16T19:45:07Z

@philsturgeon Making progress. Still one karma failing test.

nulltoken · 2019-11-17T16:16:26Z

@philsturgeon Jest and harness tests have been fixed. Still got an issue with the last one.
It fails differently on node 12 and node 10 (#737 should help fix fix by making the sorting a bit more thorough).

Feel free to cherry pick and squash the proposed changes you like (don't hate too much?) in your PR.

I'm going to decorate some of the changes I'm the less happy with with some comments.

nulltoken · 2019-11-17T16:20:14Z

src/rulesets/oas/functions/__tests__/oasOpIdUnique.test.ts

@@ -6,7 +6,6 @@ import oasOpIdUnique from '../oasOpIdUnique';

 describe('oasOpIdUnique', () => {
  const s = new Spectral();
-  s.registerFormat('oas2', () => true);


@philsturgeon I've dropped this as this is supposed to not be tied to oas2. Does that make sense? Another option would be to revert that change and add a new test targeting oas3.

I think a lot of thse registerFormat's were erroneous and this is fine.

nulltoken · 2019-11-17T16:25:50Z

src/rulesets/oas/index.json

@@ -515,7 +512,7 @@
      "message": "{{error}}",
      "recommended": true,
      "formats": ["oas2"],
-      "severity": "error",


@philsturgeon I do think "error" would be nicer but I was more focused on making the tests pass.

Proposal: Unless this is blocking, maybe log a tech debt issue and switch back to "error" in another PR?

src/cli/services/__tests__/linter.test.ts

src/rulesets/oas/index.json

src/rulesets/oas/functions/__tests__/oasTagDefined.test.ts

nulltoken · 2019-11-19T09:57:11Z

@philsturgeon rebased on top of your latest changes

philsturgeon · 2019-11-19T20:49:05Z

@nulltoken this is amazing! Shall I leave this in your capable hands? @P0lip can you help him get this over the line?

P0lip · 2019-11-19T23:17:10Z

@philsturgeon yup! will do my best to help.

nulltoken · 2019-11-20T15:19:20Z

@nulltoken this is amazing!

@philsturgeon YOU did the hard work. All I had to do was fighting against jest matchers (Yes. Looking at you expectArrayContaining and expectObjectContaining)

Shall I leave this in your capable hands? @P0lip can you help him get this over the line?

Ok. Taking over this task then.

nulltoken · 2019-11-20T17:18:53Z

@P0lip It's now rebased. I've started to reduce a bit the diff noise by removing unneeded changes.

Could you please start a first pass of review?

P0lip · 2019-11-20T18:47:05Z

src/formatters/stylish.ts

@@ -23,7 +23,7 @@
 * @author Sindre Sorhus
 */

-import chalk from 'chalk';
+import chalk = require('chalk');


Suggested change

import chalk = require('chalk');

import * as chalk 'chalk';

@P0lip StripAnsi requires the same kind "required" import but doesn't seem to like the import * syntax.

How about switching esModuleInterop flag to true? Would that mess something up?

Don't worry about it. I've already updated both of these dependencies on develop :)

How about switching esModuleInterop flag to true? Would that mess something up?

@P0lip Yep, I've seen that. But not being a transpiling guru, I'd genuinely be interested in your standpoint on that.

src/formatters/__tests__/stylish.test.ts

P0lip · 2019-11-20T18:48:06Z

src/rulesets/__tests__/reader.jest.test.ts

@@ -353,42 +336,6 @@ describe('Rulesets reader', () => {
    ).toHaveLength(0);
  });

-  it('should limit the scope of formats to a ruleset', () => {


The test should be kept. We just need to use different (custom) rulesets.

@P0lip Tentatively replaced with should limit the scope of formats to a ruleset.

Thoughts?

@P0lip I'll rewrite this to custom rulesets later today.

@P0lip Done. Did I manage to keep the intent of the original test?

Yup, seems good! Thank you.

P0lip · 2019-11-20T18:48:24Z

src/rulesets/__tests__/reader.test.ts

@@ -1,21 +1,8 @@
 import { readRuleset } from '../reader';

 describe('Rulesets reader', () => {
-  it('should resolve oas2-schema', async () => {


Ditto, guess we want to keep it

@P0lip Fixed

docs/getting-started/rulesets.md

philsturgeon · 2019-11-21T16:27:32Z

Minimise the change in order to get this one done. We don’t need to do all the things I got carried away.

…

On Thu, Nov 21, 2019 at 16:22, Jakub Rożek ***@***.***> wrote: @P0lip commented on this pull request. --------------------------------------------------------------- In [docs/getting-started/rulesets.md](#773 (comment)): > formats: ["oas3"] - given: "$" + given: "$.servers" To mitigate the risk, I'd suggest reverting these for now. We can revisit them in a separate PR. What do you think? As I described in Phil's PR, field with given vs given with no field they are not equivalent in certain circumstances. — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#773?email_source=notifications&email_token=AAAQONNZCYYUK2LEVKTTWODQU2YVVA5CNFSM4JOFQL4KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMRENJA#pullrequestreview-321013412), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAAQONMPGDP37LHEQREAHZDQU2YVVANCNFSM4JOFQL4A).

nulltoken · 2019-11-21T17:03:40Z

Copied over from #772 (comment)

cc @nulltoken since you are working on that too.
I'm pretty sure src/rulesets/oas3/tests/api-servers.ts fails now.

@P0lip I don't see it failing in #773. Or is it passing for the wrong reason? Or did I misunderstand you?

nulltoken · 2019-11-21T20:34:07Z

@P0lip I'll work on this tomorrow. Should by ready for review by Monday.

P0lip · 2019-11-22T09:36:37Z

@P0lip I don't see it failing in #773. Or is it passing for the wrong reason? Or did I misunderstand you?

It's not failing, because the rule looks the same.

    "oas3-api-servers": {
      "description": "OpenAPI `servers` must be present and non-empty array.",
      "recommended": true,
      "formats": ["oas3"],
      "given": "$",
      "then": {
        "field": "servers",
        "function": "schema",
        "functionOptions": {
          "schema": {
            "items": {
              "type": "object"
            },
            "minItems": 1,
            "type": "array"
          }
        }
      },
      "tags": [
        "api"
      ]
    },

nulltoken · 2019-11-23T09:42:33Z

Minimise the change in order to get this one done.

@philsturgeon @P0lip I've tried to reduce the diff noise as much as possible.
I'll try to get rid of some of the added registerFormat.

nulltoken · 2019-11-23T11:33:18Z

Minimise the change in order to get this one done.

@philsturgeon @P0lip I've tried to reduce the diff noise as much as possible.
I'll try to get rid of some of the added registerFormat.

@P0lip @philsturgeon Looks like the tests fail if I remove those new registerFormat() calls. However, I'm not quite sure why they're now required whereas they previously weren't.

philsturgeon · 2019-11-23T12:59:24Z

I added formats to a lot of rules whereas before they were applied at the ruleset level. The way we pluck rules and put them into setRules in our tests means this is now important. Our test stuff is a little messy but we should clean it up in another thing, let’s just get it working first.

…

On Sat, Nov 23, 2019 at 11:33, nulltoken ***@***.***> wrote: >> Minimise the change in order to get this one done. > > ***@***.***(https://github.com/philsturgeon) ***@***.***(https://github.com/P0lip) I've tried to reduce the diff noise as much as possible. > I'll try to get rid of some of the added registerFormat. ***@***.***(https://github.com/P0lip) ***@***.***(https://github.com/philsturgeon) Looks like the tests fail if I remove those new registerFormat() calls. However, I'm not quite sure why they're now required whereas they previously weren't. — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#773?email_source=notifications&email_token=AAAQONIJHQN2ZTLYCRIIKZLQVEIH5A5CNFSM4JOFQL4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE7TIOI#issuecomment-557790265), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAAQONJSSZTHZFEMXIN2R7DQVEIH5ANCNFSM4JOFQL4A).

nulltoken · 2019-11-23T21:46:10Z

I added formats to a lot of rules whereas before they were applied at the ruleset level. The way we pluck rules and put them into setRules in our tests means this is now important. Our test stuff is a little messy but we should clean it up in another thing, let’s just get it working first.

@philsturgeon Thanks for this feedback. Hower, I now realize that, as a user, I may not have clearly understood that subtlety.

The doc states Spectral supports two core formats: oas2 and oas3. Using registerFormat you can add support for autodetecting other formats. This seems to say "Hey! You're linting OAS2 or OAS3? Cool! Everything is already set up.". Or is this a cli vs Api difference in usage?
I my mind, when a ruleset was exposing formats, rules that didn't specify a format were inheriting those ruleset formats. I'm not sure to understand why specifying now a format at the rule level requires to previously invoke registerFormat. Or was that inheritance thingie all in my head only?

Considering this, when one should invoke registerFormat seems a bit "blurry" to me. Any chance you could sprinkle a bit of your magic documentation writing dust to enlighten this?

nulltoken · 2019-11-24T20:38:45Z

Rebased and cleaned up history.

P0lip · 2019-11-26T00:56:30Z

@nulltoken I'll review it today!

P0lip

Looking very well!
Almost ready to be merged.

P0lip · 2019-11-27T01:50:59Z

docs/getting-started/rulesets.md

@@ -80,7 +80,6 @@ Specifying the format is optional, so you can completely ignore this if all the
 rules:
  api-servers:


We have renamed this rule, haven't we?

Suggested change

api-servers:

oas3-api-servers:

P0lip · 2019-11-27T01:52:56Z

src/rulesets/__tests__/reader.jest.test.ts


-            return rules;
-          }, {}),
+    expect(rules['generic-valid-rule'].formats).toBeInstanceOf(Array);


Suggested change

expect(rules['generic-valid-rule'].formats).toBeInstanceOf(Array);

(Unneeded since we match against an array of oas2 and oas3)

P0lip · 2019-11-27T01:53:01Z

src/rulesets/__tests__/reader.jest.test.ts

-            rules[name] = expect.objectContaining({
-              formats: ['oas3'],
-            });
+    expect(rules['oas2-valid-rule'].formats).toBeInstanceOf(Array);


Suggested change

expect(rules['oas2-valid-rule'].formats).toBeInstanceOf(Array);

P0lip · 2019-11-27T01:53:05Z

src/rulesets/__tests__/reader.jest.test.ts

-        },
-      }),
-    );
+    expect(rules['oas3-valid-rule'].formats).toBeInstanceOf(Array);


Suggested change

expect(rules['oas3-valid-rule'].formats).toBeInstanceOf(Array);

P0lip · 2019-11-27T01:53:42Z

src/rulesets/__tests__/reader.jest.test.ts

@@ -353,42 +336,6 @@ describe('Rulesets reader', () => {
    ).toHaveLength(0);
  });

-  it('should limit the scope of formats to a ruleset', () => {


Yup, seems good! Thank you.

P0lip · 2019-11-27T01:57:27Z

src/rulesets/oas3/__tests__/model-description.ts

-import { RuleType, Spectral } from '../../../spectral';
-import * as ruleset from '../index.json';
-
-describe('components-schema-description', () => {


Have we migrated this test? Cannot find it anywhere

Dropped by @philsturgeon (as described in #725)

Ah, right. Confused the names.

P0lip · 2019-11-27T01:57:37Z

src/rulesets/oas2/__tests__/model-description.ts

-import { RuleType, Spectral } from '../../../spectral';
-import * as ruleset from '../index.json';
-
-describe('definition-description', () => {


Ditto, has it been migrated?

Dropped by @philsturgeon (as described in #725)

nulltoken · 2019-11-27T09:03:19Z

Looking very well!
Almost ready to be merged.

@P0lip I believe all comments have been dealt with.

philsturgeon · 2019-11-27T12:29:11Z

docs/guides/javascript.md

@@ -107,21 +107,22 @@ openapi: 3.0.0
 `

 const spectral = new Spectral();
-spectral.registerFormat('oas2', isOpenApiv2);
-spectral.registerFormat('oas3', isOpenApiv3);spectral.loadRuleset('spectral:oas3') // spectral:oas2 for OAS 2.0 aka Swagger
+spectral.registerFormat('oas2', isOpenApiv2); // spectral:oas2 for OAS 2.0 aka Swagger


Suggested change

spectral.registerFormat('oas2', isOpenApiv2); // spectral:oas2 for OAS 2.0 aka Swagger

spectral.registerFormat('oas2', isOpenApiv2); // spectral:oas2 for OpenAPI v2.0

We can do better than saying "oas2 = OAS 2" and we mention swagger plenty elsewhere.

Btw I know you didnt add this im just noticing it now 😅

@philsturgeon Fixed

philsturgeon · 2019-11-27T12:29:28Z

docs/guides/javascript.md

@@ -107,21 +107,22 @@ openapi: 3.0.0
 `

 const spectral = new Spectral();
-spectral.registerFormat('oas2', isOpenApiv2);
-spectral.registerFormat('oas3', isOpenApiv3);spectral.loadRuleset('spectral:oas3') // spectral:oas2 for OAS 2.0 aka Swagger
+spectral.registerFormat('oas2', isOpenApiv2); // spectral:oas2 for OAS 2.0 aka Swagger


Btw I know you didnt add this im just noticing it now 😅

P0lip

Just an extra comment.
Looks good! Thank you so much for taking care of this. Much appreciated ❤️

@philsturgeon could you have another pass too?
I did read the code thrice yet since the PR is fairly sized I might have skipped something.

P0lip · 2019-11-27T22:40:02Z

src/__tests__/linter.test.ts

-      mergeRules(oas3Rules, {
-        'valid-example-in-schemas': 'off',
+      mergeRules(oasRules, {
+        'oas3-valid-schema-example': 'off',
        'components-schema-description': -1,


Let's change this rule to something else since it's removed.

@P0lip Replaced with operation-2xx-response

P0lip · 2019-11-27T22:40:13Z

src/rulesets/oas3/__tests__/model-description.ts

-import { RuleType, Spectral } from '../../../spectral';
-import * as ruleset from '../index.json';
-
-describe('components-schema-description', () => {


Ah, right. Confused the names.

nulltoken mentioned this pull request Nov 16, 2019

One ruleset #689

Closed

7 tasks

nulltoken changed the title ~~Ntk/fixup unify~~ [NOT FOR MERGE] Ntk/fixup unify Nov 16, 2019

nulltoken commented Nov 17, 2019

View reviewed changes

nulltoken changed the base branch from develop to feat/improve-oas-rulesets November 17, 2019 16:31

nulltoken commented Nov 17, 2019

View reviewed changes

src/rulesets/oas/functions/__tests__/oasTagDefined.test.ts Outdated Show resolved Hide resolved

philsturgeon force-pushed the feat/improve-oas-rulesets branch from 5904907 to 7fcdc56 Compare November 18, 2019 14:50

nulltoken changed the title ~~[NOT FOR MERGE] Ntk/fixup unify~~ [WIP] Unify rulesets Nov 20, 2019

nulltoken changed the base branch from feat/improve-oas-rulesets to develop November 20, 2019 15:53

P0lip reviewed Nov 20, 2019

View reviewed changes

src/formatters/__tests__/stylish.test.ts Outdated Show resolved Hide resolved

P0lip reviewed Nov 20, 2019

View reviewed changes

nulltoken mentioned this pull request Nov 21, 2019

Improving and unifying oas ruleset #772

Closed

4 tasks

P0lip reviewed Nov 21, 2019

View reviewed changes

docs/getting-started/rulesets.md Outdated Show resolved Hide resolved

nulltoken changed the title ~~[WIP] Unify rulesets~~ feat: drop oas2 and oas3 rulesets Nov 24, 2019

P0lip reviewed Nov 27, 2019

View reviewed changes

philsturgeon reviewed Nov 27, 2019

View reviewed changes

philsturgeon previously approved these changes Nov 27, 2019

View reviewed changes

nulltoken dismissed philsturgeon’s stale review via 111dc29 November 27, 2019 19:32

P0lip previously approved these changes Nov 27, 2019

View reviewed changes

Phil Sturgeon added 2 commits November 28, 2019 09:15

removed operation-summary-formatted and changed some reccomends

5ede4a1

attempting to unify oas, oas2 and oas3

5b4977d

nulltoken dismissed P0lip’s stale review via 5b4977d November 28, 2019 08:16

P0lip approved these changes Nov 28, 2019

View reviewed changes

P0lip added the breaking Pull requests that introduce a breaking change label Nov 28, 2019

P0lip assigned nulltoken Nov 28, 2019

philsturgeon approved these changes Dec 2, 2019

View reviewed changes

Merge branch 'develop' into ntk/fixup_unify

eb9f24a

P0lip merged commit 84a8238 into stoplightio:develop Dec 2, 2019

This was referenced Dec 2, 2019

Remove 'summary' field from any default rules #771

Closed

Clear out old TS rules and functions #561

Closed

nulltoken deleted the ntk/fixup_unify branch December 5, 2019 18:35

		@@ -80,7 +80,6 @@ Specifying the format is optional, so you can completely ignore this if all the
		rules:
		api-servers:

	spectral.registerFormat('oas2', isOpenApiv2); // spectral:oas2 for OAS 2.0 aka Swagger
	spectral.registerFormat('oas2', isOpenApiv2); // spectral:oas2 for OpenAPI v2.0

feat: drop oas2 and oas3 rulesets #773

feat: drop oas2 and oas3 rulesets #773

Conversation

nulltoken commented Nov 16, 2019 • edited Loading

nulltoken commented Nov 16, 2019

nulltoken commented Nov 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented Nov 19, 2019

philsturgeon commented Nov 19, 2019

P0lip commented Nov 19, 2019

nulltoken commented Nov 20, 2019 • edited Loading

nulltoken commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philsturgeon commented Nov 21, 2019 via email

nulltoken commented Nov 21, 2019

nulltoken commented Nov 21, 2019

P0lip commented Nov 22, 2019

nulltoken commented Nov 23, 2019

nulltoken commented Nov 23, 2019

philsturgeon commented Nov 23, 2019 via email

nulltoken commented Nov 23, 2019 • edited Loading

nulltoken commented Nov 24, 2019

P0lip commented Nov 26, 2019

P0lip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented Nov 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P0lip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented Nov 16, 2019 •

edited

Loading

nulltoken commented Nov 17, 2019 •

edited

Loading

nulltoken commented Nov 20, 2019 •

edited

Loading

nulltoken commented Nov 23, 2019 •

edited

Loading