Skip to content

Conversation

@philsturgeon
Copy link
Contributor

@philsturgeon philsturgeon commented Nov 15, 2018

skip has been broken since v0.8, and this branch is attempting to fix it.

The current implementation is setting a variable scoped to the linter module, but that gets wiped out and is prone torace conditions, which is why the test suite is failing on this PR.

Another approach I attempted was converting the linter to be an ES6 class so I could instantiate different instances for the test suite, but that is a large divergence from oas-kit and we're meant to be getting closer not further apart.

@MikeRalphson at this point I'm wondering if you can use your superior intelligence and coding abilities to see the use case here and add a feature to oas-linter, so I can just delete my linter and tests and use that.

Maybe I can bribe you at API Days? :D

@MikeRalphson
Copy link
Contributor

@philsturgeon working on this today!

@MikeRalphson
Copy link
Contributor

@philsturgeon what kind of interface do you need for skipping linter rules? I've added a simple array options.lintSkip which takes rule instance names and just walks past them if they're present. Is that sufficient? It's in the v4linter branch.

@philsturgeon
Copy link
Contributor Author

@MikeRalphson that should work! I'll give it a try with that branch. Basically so long as I can pass something from the CLI to the linter (ofc going via the validator), and that thing won't get too upset if there are multiple linters running at the same time, then we're all set.

@philsturgeon
Copy link
Contributor Author

@MikeRalphson one of the remaining differences between the two linters is how we add lint rules. We have a loader which handles all file related loading. It's a bit of a mess and there's some daft duplication that could be refactored, but it handles loading the rules files instead of having loading logic directly in the linter.

https://github.com/wework/speccy/blob/master/lib/loader.js#L131

Doing this means we can add rules dynamically, which makes for some good testing, and theoretically better separation of concerns.

The only other difference is relevantRules which doesnt matter, as its main job is to skip stuff and your skip logic takes care of that.

@MikeRalphson
Copy link
Contributor

So if I split my file loader into a loader and a parser for setting up new rules, will that be enough?

@philsturgeon
Copy link
Contributor Author

philsturgeon commented Dec 8, 2018 via email

@MikeRalphson
Copy link
Contributor

Cool.

@pderaaij
Copy link
Contributor

pderaaij commented Jan 6, 2019

@philsturgeon I have done a simple try at making use of oas-linter for linting and removing all the current lint logic in speccy. This is the direction you'd like to take right?

Just for insight you can have a look at https://github.com/pderaaij/speccy/pull/1/files for what I have done.

If this is the right path I will work on fixing tests and make things proper.

@pderaaij pderaaij mentioned this pull request Jan 8, 2019
@philsturgeon
Copy link
Contributor Author

Superseded by #248

@philsturgeon philsturgeon deleted the bug/skip branch January 18, 2019 16:34
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.

4 participants