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

Allow piping into semver to read versions #161

Closed
wants to merge 5 commits into from
Closed

Allow piping into semver to read versions #161

wants to merge 5 commits into from

Conversation

richardgarnier
Copy link

This is a patch for #149.

Before entering the main, we check if the process is piped. If so then we fill the versions array and then we launch the main. In order to fill the array, we split on spaces.

NB : versions can be accepted both from the pipe and as arguments.
For example echo 1.0.0 | semver -r '>=1' 1.1.1 is accepted.

NB2 : testing relies on comparing the output of the version without piping, and the version with piping.

@othiym23 othiym23 added the review label Jun 4, 2016
@coveralls
Copy link

coveralls commented Jun 4, 2016

Coverage Status

Coverage remained the same at 94.624% when pulling 0841679 on akatsukle:master into 8bd070b on npm:master.

@@ -56,6 +75,7 @@ function main () {
break
case "-h": case "--help": case "-?":
return help()
break
Copy link
Author

Choose a reason for hiding this comment

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

I guess the break here was missing. This is not the main purpose of the patch though.

Copy link
Contributor

@isaacs isaacs Jun 23, 2016

Choose a reason for hiding this comment

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

A break isn't strictly necessary here, since the return is just as effective at ending the block. But if a linter or something is complaining about it, whatever.

@coveralls
Copy link

coveralls commented Jun 4, 2016

Coverage Status

Coverage remained the same at 94.624% when pulling e63742a on akatsukle:master into 8bd070b on npm:master.

@coveralls
Copy link

coveralls commented Jun 4, 2016

Coverage Status

Coverage remained the same at 94.624% when pulling d4ca6fb on akatsukle:master into 8bd070b on npm:master.

@coveralls
Copy link

coveralls commented Jun 4, 2016

Coverage Status

Coverage remained the same at 94.624% when pulling d149eb4 on akatsukle:master into 8bd070b on npm:master.

@richardgarnier
Copy link
Author

Well that test was embarrassing... sorry

@isaacs
Copy link
Contributor

isaacs commented Jun 29, 2016

This is ok, and seems to work, but it seems like it'd be better to open stdin, and parse/filter each version as lines are read. That way it could be done interactively. I could see this being really handy if you want to test out whether a given range works with various versions.

You can use node's builtin readline module to do this.

@isaacs
Copy link
Contributor

isaacs commented Feb 8, 2017

Oh, I just realized something about this.

This means that any programs that spawn('semver') are going to hang until stdin is closed.

That's probably surprising. One approach would be to only read the stdin pipe if - is passed as an argument.

@settings settings bot removed the review label Dec 14, 2019
@farahmandakbar farahmandakbar linked an issue May 12, 2022 that may be closed by this pull request
@lukekarrys lukekarrys added the cli referencing the semver cli label Oct 27, 2022
@lukekarrys lukekarrys mentioned this pull request Oct 27, 2022
1 task
@wraithgar
Copy link
Member

Closing this one for now since it still has work to be done.

@wraithgar wraithgar closed this Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli referencing the semver cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

require('semver')
6 participants