Skip to content
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: support for stdin and piping #270

Merged
merged 3 commits into from
Mar 8, 2019

Conversation

dlouzan
Copy link
Contributor

@dlouzan dlouzan commented Jan 26, 2019

  • validate properly flushes output so pipes stdout works
  • lint accepts as input stdin (and pipe flows)
  • move all verbose option messages to stderr so that pipes also work with verbose output

Fixes #235

- validate properly flushes output so pipes stdout works
- lint accepts as input stdin (and pipe flows)
- move all verbose option messages to stderr so that pipes also
  work with verbose output
@dlouzan
Copy link
Contributor Author

dlouzan commented Jan 26, 2019

A couple of comments about the code:

  • This allows a flow like:
    speccy resolve <file> | speccy lint
    # also
    speccy lint < <file>
  • I've moved all non data output from stdout to stderr. This is the standard for error messages, debug info, warnings, etc, at least in the *nix world
  • There's still a couple of messages that seem to come from oas-validator when activating the verbose mode. I'll open an issue in there. Those prevent e.g. to be able to activate verbose mode and pipe the output of resolve to lint.
  • In resolve.js I did some refactoring of the promise returning the output, I think it's better style to have a clear branch path for each reject/resolve, instead of returning early :-)
  • On the tests, replaced the use of jest.resetAllMocks to restoreAllMocks, which has different semantics, and actually solves the issue that the output of each test was lost after the first call

@dlouzan
Copy link
Contributor Author

dlouzan commented Jan 26, 2019

Disclaimer: I haven't tested it in Windows, I don't really have a setup available for it :-)

@dlouzan
Copy link
Contributor Author

dlouzan commented Jan 28, 2019

This is of course also possible:

(feat/stdin-support= dd9713a)$ ./speccy.js lint <<EOF
> openapi: 3.0.0
> info:
>   contact:
>     name: Derp
>     email: derp@herp.com
>   version: 1.0.0
>   title: Swagger 2.0 Without Scheme
> paths: {}
> tags:
>   - name: Gym
>   - name: Pokemon
> EOF
Specification is valid, with 0 lint errors

@pderaaij
Copy link
Contributor

Looks good to me, thanks for your contribution!

@MikeRalphson
Copy link
Contributor

Successfully published:

  • oas-linter@3.0.1
  • oas-resolver@2.2.1
  • oas-validator@3.2.1

@dlouzan
Copy link
Contributor Author

dlouzan commented Feb 21, 2019

May I ping you guys on this? The PR is already a month old but seems the project is a bit stalled and building a stack of PR's. I'm currently working from a fork of this but integrating the functionality directly in the published package would be great :-)

Same goes for the fixes done by @MikeRalphson in Mermade/oas-kit#152 and (potentially if you agree with it) #294.

Thanks!

@pderaaij
Copy link
Contributor

I can't merge PRs unfortunately, we have to ping @djtarazona or @smythey21 for merging PRs.

@djtarazona
Copy link
Contributor

Hey team, I will review this no later than this weekend. Sorry for the slowness on our side — we are working on getting better OSS processes in place and expect things to pick up soon.

@djtarazona
Copy link
Contributor

@dlouzan LGTM, and works in my local smoke testing. Can you rebase and fix any conflicts? Then I'll get this merged in. Thanks!

Solve conflicts with latest master changes
@dlouzan
Copy link
Contributor Author

dlouzan commented Mar 8, 2019

I have just updated the branch, it was just some conflicts with the latest changes to package versions.

@djtarazona djtarazona merged commit c6621f8 into wework:master Mar 8, 2019
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