-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
feat: drop oas2 and oas3 rulesets #773
Conversation
@philsturgeon Making progress. Still one karma failing test. |
@philsturgeon Jest and harness tests have been fixed. Still got an issue with the last one. 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. |
@@ -6,7 +6,6 @@ import oasOpIdUnique from '../oasOpIdUnique'; | |||
|
|||
describe('oasOpIdUnique', () => { | |||
const s = new Spectral(); | |||
s.registerFormat('oas2', () => true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a lot of thse registerFormat's were erroneous and this is fine.
src/rulesets/oas/index.json
Outdated
@@ -515,7 +512,7 @@ | |||
"message": "{{error}}", | |||
"recommended": true, | |||
"formats": ["oas2"], | |||
"severity": "error", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
5904907
to
7fcdc56
Compare
@philsturgeon rebased on top of your latest changes |
@nulltoken this is amazing! Shall I leave this in your capable hands? @P0lip can you help him get this over the line? |
@philsturgeon yup! will do my best to help. |
@philsturgeon YOU did the hard work. All I had to do was fighting against jest matchers (Yes. Looking at you
Ok. Taking over this task then. |
@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? |
src/formatters/stylish.ts
Outdated
@@ -23,7 +23,7 @@ | |||
* @author Sindre Sorhus | |||
*/ | |||
|
|||
import chalk from 'chalk'; | |||
import chalk = require('chalk'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import chalk = require('chalk'); | |
import * as chalk 'chalk'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about it. I've already updated both of these dependencies on develop :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -353,42 +336,6 @@ describe('Rulesets reader', () => { | |||
).toHaveLength(0); | |||
}); | |||
|
|||
it('should limit the scope of formats to a ruleset', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should be kept. We just need to use different (custom) rulesets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip Tentatively replaced with should limit the scope of formats to a ruleset
.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip I'll rewrite this to custom rulesets later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip Done. Did I manage to keep the intent of the original test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, seems good! Thank you.
@@ -1,21 +1,8 @@ | |||
import { readRuleset } from '../reader'; | |||
|
|||
describe('Rulesets reader', () => { | |||
it('should resolve oas2-schema', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, guess we want to keep it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip Fixed
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).
|
Copied over from #772 (comment)
@P0lip I don't see it failing in #773. Or is it passing for the wrong reason? Or did I misunderstand you? |
@P0lip I'll work on this tomorrow. Should by ready for review by Monday. |
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"
]
}, |
@philsturgeon @P0lip I've tried to reduce the diff noise as much as possible. |
@P0lip @philsturgeon Looks like the tests fail if I remove those new |
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).
|
@philsturgeon Thanks for this feedback. Hower, I now realize that, as a user, I may not have clearly understood that subtlety.
Considering this, when one should invoke |
Rebased and cleaned up history. |
@nulltoken I'll review it today! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very well!
Almost ready to be merged.
docs/getting-started/rulesets.md
Outdated
@@ -80,7 +80,6 @@ Specifying the format is optional, so you can completely ignore this if all the | |||
rules: | |||
api-servers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have renamed this rule, haven't we?
api-servers: | |
oas3-api-servers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
return rules; | ||
}, {}), | ||
expect(rules['generic-valid-rule'].formats).toBeInstanceOf(Array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(rules['generic-valid-rule'].formats).toBeInstanceOf(Array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Unneeded since we match against an array of oas2 and oas3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
rules[name] = expect.objectContaining({ | ||
formats: ['oas3'], | ||
}); | ||
expect(rules['oas2-valid-rule'].formats).toBeInstanceOf(Array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(rules['oas2-valid-rule'].formats).toBeInstanceOf(Array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
}, | ||
}), | ||
); | ||
expect(rules['oas3-valid-rule'].formats).toBeInstanceOf(Array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(rules['oas3-valid-rule'].formats).toBeInstanceOf(Array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -353,42 +336,6 @@ describe('Rulesets reader', () => { | |||
).toHaveLength(0); | |||
}); | |||
|
|||
it('should limit the scope of formats to a ruleset', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, seems good! Thank you.
import { RuleType, Spectral } from '../../../spectral'; | ||
import * as ruleset from '../index.json'; | ||
|
||
describe('components-schema-description', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we migrated this test? Cannot find it anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped by @philsturgeon (as described in #725)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. Confused the names.
import { RuleType, Spectral } from '../../../spectral'; | ||
import * as ruleset from '../index.json'; | ||
|
||
describe('definition-description', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, has it been migrated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped by @philsturgeon (as described in #725)
@P0lip I believe all comments have been dealt with. |
docs/guides/javascript.md
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw I know you didnt add this im just noticing it now 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philsturgeon Fixed
docs/guides/javascript.md
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw I know you didnt add this im just noticing it now 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/__tests__/linter.test.ts
Outdated
mergeRules(oas3Rules, { | ||
'valid-example-in-schemas': 'off', | ||
mergeRules(oasRules, { | ||
'oas3-valid-schema-example': 'off', | ||
'components-schema-description': -1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change this rule to something else since it's removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@P0lip Replaced with operation-2xx-response
import { RuleType, Spectral } from '../../../spectral'; | ||
import * as ruleset from '../index.json'; | ||
|
||
describe('components-schema-description', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. Confused the names.
#689 Has been superseded by #772
This PR superseds #772
This is an attempt at helping get all the tests pass.