Skip to content

Removes custom linter #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

Merged
merged 10 commits into from
Jan 18, 2019
Merged

Conversation

pderaaij
Copy link
Contributor

@pderaaij pderaaij commented Jan 8, 2019

This PR removes the custom linting logic utilizing oas-linteras linting mechanism. Also, it fixes skipping of rules via CLI.

It continues on the amazing work done by @philsturgeon in #204, but I had issues merging my changes successfully in that branch. Opted to started fresh in a new branch.

I've done a couple of tests and it works quite well. Main issue right now is that behavior of oas-linter is to load its own rule definition by default. This definition differs from the current definition by speccy and causes for different results. This has been discussed in Mermade/oas-kit#129, but behavior will change after a new release.

@pderaaij
Copy link
Contributor Author

pderaaij commented Jan 8, 2019

Note, I have some work left in the validate function to figure out

@pderaaij
Copy link
Contributor Author

pderaaij commented Jan 9, 2019

Figured out the last bits in the validate function. Think it does what it should be doing, but please check as well if it has the same results as before.

@pderaaij
Copy link
Contributor Author

pderaaij commented Jan 9, 2019

Also, this change was relatively easy thanks to the work prior done by @philsturgeon in #204 and @MikeRalphson in all the changes in oas-kit. You guys rock!

@chris-branch
Copy link

chris-branch commented Jan 14, 2019

Three things:

  1. There was a notEqual rule-type in speccy that got lost with the move to the oas linter.
    https://github.com/wework/speccy/blob/master/lib/linter.js#L136
    The default-and-example-are-redundant rule in default.yaml refers to this rule type.

  2. parameter-name-regex, pathItem-summary-or-description, and reference-components-regex rules in the default.yaml ruleset were all changed from "enabled = false" to "disabled = false" (i.e., was disabled, now is enabled). Was this intentional?

  3. The oas-linter automatically loads a rules.yaml file of its own regardless of what you request via the speccy command line. oas-linter/index.js:

loadRules(path.join(__dirname,'rules.yaml'));

@pderaaij
Copy link
Contributor Author

Regarding your questions/remarks.

  1. Good catch, I'll have a look today at adding it to oas-kit. Added issue Add notEqual rule type Mermade/oas-kit#138 to oas-kit

  2. @philsturgeon might have a better answer to this. I've used the same ruleset as mentioned in PR [WIP] Rewriting Skip Logic #204

  3. This is correct, has been addressed in Prevent oas-linter from loading own ruleset Mermade/oas-kit#129 and is awaiting release of v5 of the linter. Which is also one of the reasons this PR isn't merged yet

@pderaaij
Copy link
Contributor Author

I found a rule for the redundant fields rule. It does require a change to oas-validator. I'll do a PR to take it along in the v5 release.

@philsturgeon
Copy link
Contributor

@pderaaij we can remove pathItem-summary-or-description as it looks like it was disabled for a good reason. e02d70d

@pderaaij
Copy link
Contributor Author

Removed the rule from the default set

@philsturgeon
Copy link
Contributor

When #252 is merged grab it, and CircleCI will no longer fail.

@chris-branch
Copy link

Found one more thing: buildValidatorOptions() in lint.js needs to set linterResults:

const buildValidatorOptions = (skip, verbose) => {
    return {
      ....
        linterResults: linter.getResults,
      ....
    };
}

@pderaaij
Copy link
Contributor Author

Correct! somehow I forgot to commit it yesterday. Will do so later today. Thanks for looking into it @chris-branch !

@pderaaij pderaaij force-pushed the remove-custom-linter branch from 4d774d4 to 95fd6a6 Compare January 16, 2019 17:27
@pderaaij
Copy link
Contributor Author

I would like to have a go at some integration tests, but I will await #250 to be merged.

@philsturgeon
Copy link
Contributor

@pderaaij hey, #250 has been merged. Sorry for the conflicts, but seeing as the tests are mostly just moved this should not be too awful. It is important that we get more integration tests in, so I think the extra work will be worthwhile.

Feel free to put your tests in test/integration/foo.test.js and they wont conflict with the tests im writing. I've only covered ./server.js so far, and I'll check in with you before I write up too much more.

Keep at it, you're doing a fantastic job,.

@pderaaij pderaaij force-pushed the remove-custom-linter branch from d91df00 to 36fac38 Compare January 16, 2019 19:06
@pderaaij
Copy link
Contributor Author

No worries about #250, I'll fix it accordingly. Will probably later this week and hopefully it includes the v5 release of oas-kit as well.

@pderaaij
Copy link
Contributor Author

pderaaij commented Jan 17, 2019

@philsturgeon I had a go at adding integration test to speccy. Came quite far, see bb066fe for my changes.

Unfortunately the integration tests breaks currently, but that will be fixed when v5 of oas-kit is released.

I had to add a promise in lint.js around the validator.validate call and move the process.exit calls to speccy.js. Please have a look to see if I am doing the right thing here.

@philsturgeon
Copy link
Contributor

@pderaaij looks good to me! process.exit shouldn't be any deeper than speccy.js you're quite right. That's a mistake I've made in all the command files.

@pderaaij
Copy link
Contributor Author

Cool! I will fix in in the other commands as well. Wait for the v5 release and squash some commits. After that it should be ready for review and merge.

@pderaaij pderaaij force-pushed the remove-custom-linter branch 3 times, most recently from 4a33085 to 20cc2e5 Compare January 17, 2019 14:53
@pderaaij pderaaij force-pushed the remove-custom-linter branch from 20cc2e5 to 88fb4a5 Compare January 18, 2019 07:18
@pderaaij
Copy link
Contributor Author

@philsturgeon @chris-branch After updating the oas packages tests went green again.

Please have a look at the PR.

Copy link
Contributor

@philsturgeon philsturgeon left a comment

Choose a reason for hiding this comment

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

Ready to go after the two minor rule changes I mentioned

@pderaaij pderaaij force-pushed the remove-custom-linter branch from 7703a98 to 873611c Compare January 18, 2019 14:40
@pderaaij
Copy link
Contributor Author

@philsturgeon rules changed and rebased with latest commits

@MikeRalphson
Copy link
Contributor

@pderaaij @philsturgeon oas-validator v3.0.1 with a bit of a brown-paper-bag-over-the-head bugfix has just been published!

@pderaaij
Copy link
Contributor Author

@MikeRalphson bumped the version, thanks for notifying!

@philsturgeon philsturgeon merged commit c826b76 into wework:master Jan 18, 2019
@philsturgeon
Copy link
Contributor

Amazing work @pderaaij, thank you so much!

@gsong
Copy link

gsong commented Feb 22, 2019

@philsturgeon It seems inline --skip works, however, a simple speccy.yaml does not work:

lint:
  skip:
    - info-contact

@pderaaij pderaaij deleted the remove-custom-linter branch May 22, 2019 05:53
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.

5 participants