show only versions newer than NVM_MIN if set #3277
Conversation
|
For example, running 
|
ryenus
force-pushed
the
minver
branch
2 times, most recently
from
a81e16a
to
b10554a
Compare
There was a problem hiding this comment.
I like that it's done in awk :-) it'd be great to benchmark the difference to make sure this isn't slowing anything down meaningfully.
this will need tests, and also it'd be good to add a --min option to all the things that talk to the mirror (nvm ls-remote, nvm install, nvm version-remote) and then maybe nvm ls for good measure (altho that can be a follow-on if you like)
ljharb
added
installing node
|
Performance-wise there's not really much change or overhead, you can trust that awk is fast :-) Sometimes it might even run faster, esp. when many older versions are skipped. Meanwhile the other sibling functions and even the tests are something I'm not really familiar with, may you can chime in here? @ljharb |
|
Sure - for the tests, there's existing tests for For adding an option, if you grep for |
ljharb
force-pushed
the
master
branch
2 times, most recently
from
c6cfc3a
to
c20db2a
Compare
ryenus
force-pushed
the
minver
branch
2 times, most recently
from
abde015
to
132b9fb
Compare
|
@ljharb just added a test, however Travis CI seems stuck, any insight? |
|
BTW, I had to adjust the travis job a bit, due to the error See the screenshot below (https://app.travis-ci.com/github/nvm-sh/nvm/jobs/623514514): 
|
show only versions newer than NVM_MIN_VER if set
ryenus
changed the title
| @@ -1820,12 +1820,14 @@ nvm_print_versions() { | |||
| fi | |||
|
|
|||
| command awk \ | |||
| -v remote_versions="$(printf '%s' "${1-}" | tr '\n' '|')" \ | |||
| -v remote_versions="$(printf '%s' "${1-}" | tr '\n' '|')" -v min_ver="${NVM_MIN:-v0}" \ | |||
There was a problem hiding this comment.
is defaulting it to v0 the right choice, as opposed to skipping the logic entirely?
There was a problem hiding this comment.
Surely we could do that, meanwhile versions like v0, v0.0, and 0.0.0 (without the prefix v), are all legit, therefore, we have the filter_on flag which is deduced with this:
filter_on = (vcmp("v0.0.0", min_ver) != 0);
And given that the versions list is already sorted, once we encounter the version greater or equal to the min version specified, we turn off the filtering logic and show all versions onward.
| @@ -1863,8 +1872,7 @@ BEGIN { | |||
| fmt_version = fmt_installed; | |||
| } | |||
| padding = (!has_colors && is_installed) ? "" : " "; | |||
| padding = (is_installed && !has_colors) ? "" : " "; | |||
There was a problem hiding this comment.
nbd, but why flip these conditions?
There was a problem hiding this comment.
Not really flipping, just swapping the LHS and RHS of the && operator. This slightly improves the clarity and makes it clear that only !has_colors is the negating part. This is something I should have done in the first place.
| # versions lower than 18 should be filtered out, but v16.20.2 should be kept since it's installed | ||
| OUTPUT="$(NVM_NO_COLORS=1 NVM_MIN=18 nvm_print_versions "$(nvm_remote_versions)" | sed -r 's/^[ \t]+//')" |
There was a problem hiding this comment.
what if you have v16.20.1 installed, for example, and you still want to see that v16.20.2 is both available, and the latest LTS from that line?
maybe anything that's installed plus ^ of it should still be shown?
or maybe, we should skip the "keep if installed" logic entirely?
There was a problem hiding this comment.
I think the chosen behavior is slightly more preferable, due to the fact that nvm ls-remote has always been highlighting the installed versions, so that people can intuitively compare the versions they installed, to the newer versions available. And that kind of intuition is needed for people to decide whether or when to install a new version, which is much nicer than having to run both nvm ls-remote and nvm ls.
There was a problem hiding this comment.
ok - in that case, setting a MIN_VER of 18, with 16.20.1 installed as the only 16, should still show 16.20.2 as an installable version.
There was a problem hiding this comment.
Hmmm, isn't that a bit too far? If we do that then it means the min version is effectively the oldest installed version, hence defeating the point of specifying a min version. Nevertheless I agree it's a bit tricky to decide on which is the sweet spot.
Another option could be that we first let it out and see the user perception from the community.
since nvm is all about versions, so no need for the explicit suffix.
ryenus
force-pushed
the
minver
branch
2 times, most recently
from
46690fd
to
daad693
Compare
This is to fix #3250.