-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
Note, I have some work left in the validate function to figure out |
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. |
Also, this change was relatively easy thanks to the work prior done by @philsturgeon in #204 and @MikeRalphson in all the changes in |
Three things:
|
Regarding your questions/remarks.
|
I found a rule for the redundant fields rule. It does require a change to |
Removed the rule from the default set |
When #252 is merged grab it, and CircleCI will no longer fail. |
Found one more thing:
|
Correct! somehow I forgot to commit it yesterday. Will do so later today. Thanks for looking into it @chris-branch ! |
4d774d4
to
95fd6a6
Compare
I would like to have a go at some integration tests, but I will await #250 to be merged. |
@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 Keep at it, you're doing a fantastic job,. |
d91df00
to
36fac38
Compare
No worries about #250, I'll fix it accordingly. Will probably later this week and hopefully it includes the v5 release of |
@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 I had to add a promise in |
@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. |
Cool! I will fix in in the other commands as well. Wait for the |
4a33085
to
20cc2e5
Compare
20cc2e5
to
88fb4a5
Compare
@philsturgeon @chris-branch After updating the oas packages tests went green again. Please have a look at the PR. |
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.
Ready to go after the two minor rule changes I mentioned
7703a98
to
873611c
Compare
@philsturgeon rules changed and rebased with latest commits |
@pderaaij @philsturgeon |
@MikeRalphson bumped the version, thanks for notifying! |
Amazing work @pderaaij, thank you so much! |
@philsturgeon It seems inline
|
This PR removes the custom linting logic utilizing
oas-linter
as 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.