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

Remove prev, iojs alias, add quiet mode and more. #256

Closed
wants to merge 9 commits into from
Closed

Remove prev, iojs alias, add quiet mode and more. #256

wants to merge 9 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 1, 2015

Some changes to the script, readme file and package.json file.

  • Pulled prev from the script and documentation as discussed in issue remove "prev" command in 1.3 #184.
  • Pulled the iojs alias from the script and documentation because it never worked as described in issue iojs alias does nothing #255.
  • Added a quiet mode option (-q or --quiet) for curl to not display the progress bar as requested in issue Optional Progress Bar #205.
    • Fixed a "bug" in the install function where curl was hard coded to get the tarball instead of what is set in the GET variable.
    • I also moved the call to tarball_url in the install function below the test for an already installed version. It is a waste to build the url every time and not use it.
  • I pulled documentation for n io stable and n io --stable but they still work. They actually work correctly now for io by always calling latest. This was brought up in issue n io stable will install io-v1.2.0 instead of v1.3.0 #230 and I sat on it for a bit. I decided that this would be best for now because it provides backward compatibility.

I also added some minor items from other PRs:
#241 - Added Gitter badge
#239 - Calling n bin would not advance the shell prompt to a new line. I just switched printf to echo.

One final item I pulled is the node keyword which called set_default. It looks like noise because the default state for n would be to install node. I also need the keyword for a feature request in issue #185. To preserve script state I had to add separate functions to install latest and stable so it remembers which mirror to use.

And as @tjwebb said in issue #184 the version was changed to 1.4.0 in the script and package.json files. Or should it be 2.0.0? And if we go to version 2.0.0, maybe I should just make calls to n io (--)stable abort?

@maxrimue
Copy link
Contributor

maxrimue commented Apr 2, 2015

👍 Looks good!
I think we should go to 2.0.0 since these changes are "incompatible API changes" and therefore are major changes.

@ghost
Copy link
Author

ghost commented Apr 2, 2015

Just updated the PR. Put version to 2.0.0 and using n io stable or n io --stable will now abort and report an error because it is not a supported command anymore.

@knownasilya
Copy link
Contributor

Can we pull out --quiet into a separate PR so it can get in quicker (before major bump)?

@ghost
Copy link
Author

ghost commented Apr 15, 2015

@tjwebb I merged upstream to clear any conflicts. What is your take on this PR?

@tjwebb
Copy link
Collaborator

tjwebb commented Apr 16, 2015

I wish it were only one feature. It'll take a little awhile to review

@troy0820
Copy link
Collaborator

troy0820 commented May 7, 2015

Are we jumping from 1.3.0 to 2.0.0?

@maxrimue
Copy link
Contributor

maxrimue commented May 7, 2015

@troy0820 If we really modify n that much, we should
@tjwebb Is there any progress on the review btw?

@knownasilya
Copy link
Contributor

I say split it out into several PRs, ones that require a major bump and ones that don't.

@tjwebb
Copy link
Collaborator

tjwebb commented May 7, 2015

@tedgaydos yea as I mentioned, a PR should contain one thing. No progress on the review, haven't had time. It'll be easier to review if it were multiple PRs, and I agree with @knownasilya

@qw3rtman
Copy link
Collaborator

qw3rtman commented May 7, 2015

This is a major API change. If we merge these changes, we should update to 2.0.0. 👍

@troy0820
Copy link
Collaborator

troy0820 commented May 7, 2015

I agree with @knownasilya and @tjwebb. Multiple pull requests that can eventually push this to 2.0.0.

@ghost
Copy link
Author

ghost commented May 7, 2015

I will break it up tonight.

@ghost
Copy link
Author

ghost commented May 8, 2015

Everything is in separate PRs now. See #267, #268, #269, #270, #271

@knownasilya
Copy link
Contributor

@tedgaydos very nice!

@ghost
Copy link
Author

ghost commented May 8, 2015

@tjwebb It is separated into new PRs. I am closing this one.

@ghost ghost closed this May 8, 2015
This pull request was closed.
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