Responses to PRs #48
Responses to PRs #48
Conversation
|
If any of you WONDERFUL PEOPLE want to test this before it goes live, I'll give this a day before I merge. |
| # get the latest stable version number of nvm from the repo's homepage | ||
| [ "$STABLE" == "" ] && STABLE=$(curl -s -k https://github.com/creationix/nvm/ | grep "curl https://raw.githubusercontent.com/creationix/nvm/" | grep -oE "v\d+\.\d+\.\d+") | ||
| [[ $STABLE =~ ^v[0-9]+.[0-9]+.[0-9]+$ ]] || STABLE="v0.25.1" | ||
| # get the latest stable version number of nvm from the repo's package.json |
There was a problem hiding this comment.
This is very brittle. The previous approach is the one you should be using (getting a list of tags, sorting, and grabbing the last one).
There was a problem hiding this comment.
Thanks for taking the time to review this, @ljharb.
The previous version was just checking the README for the version, whereas this version gets the package.json and parses the JSON, which is arguably a bit safer than parsing a Markdown.
Having said that, I prefer the idea of fetching the tags on your repo and getting the latest stable version.
I'll look into this as an option. We are using your curl instructions now to just pull down the installer, but I think we could clone the repo and checkout the latest stable tag.
Thanks for the feedback.
There was a problem hiding this comment.
Certainly parsing the readme is brittle; using git tag output is robust.
I apologize to all of you that submitted Issues and Pull Requests. You are all WONDERFUL people and you should feel good about yourselves, because I called my mom and specifically asked her about each of you and she call YOUR moms and reported back that you all are, IN FACT, WONDERFUL PEOPLE!
This PR will hopefully address some of the many issues that have been raised. But I'm pretty good at breaking things, and in my tests, yes, there were a few rare cases that did break, but for the most part things worked, so I'm shipping it.
VERSION 0.0.18 includes the following, and please let me know if any of you have any issues after this version:
#38 Capture output when Node isn't found - Thank you @bladnman
#45 Handle Arch users (insert Reddit jab here), but seriously, thank you @peternann
#46 I took your advice and am getting the nvm version from their package.json - thanks @mlbrgl