show only versions newer than NVM_MIN if set#3277
show only versions newer than NVM_MIN if set#3277ryenus wants to merge 1 commit intonvm-sh:masterfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
a81e16a to
b10554a
Compare
ljharb
left a comment
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)
|
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 |
c6cfc3a to
c20db2a
Compare
abde015 to
132b9fb
Compare
|
@ljharb just added a test, however Travis CI seems stuck, any insight? |
46690fd to
daad693
Compare
3ad81e7 to
33e1b3d
Compare
ljharb
left a comment
There was a problem hiding this comment.
This looks great, I'll play with it locally a bit this week.
|
When I run When I run |
|
I'm certain that I'm using the code from this PR, yes. I'm running this on a Mac; if you're not, then perhaps there's some platform-specific difference here? |
|
hm, ok i did just figure out that i had a |
|
ok, testing again, and What OS are you using exactly? |
|
ohhh wait, lol i think this is the actual feature i asked for - i have tons of node versions installed, going back a very long way: $ NVM_MIN=4 nvm ls-remote | wc -l
711
$ nvm ls-remote --min=4 | wc -l
711
$ nvm ls-remote | wc -l
790in other words, it's filtering some things out, but with the additional logic i requested, it's still showing me a lot of things i didn't expect. 🤔 |
|
@ljharb FYI, just rebased with some minor fix in tests. |
|
No progress yet; I've been traveling. I'm prioritizing a bug fix on nvm before this PR, but it'll be next in line after that. |
|
ok, so i just tried it out again. if i do I'm starting to wonder if perhaps my original concern - about versions being hidden - is better addressed by prepending a message like "Minimum version X set: N versions hidden", and going back to the simpler logic? (i'm super sorry if this means you're doing work to undo the work i already asked you to do :-/ unfortunately sometimes it takes time and first-hand experience to really get a sense of the DX/UX of a thing) |
This comment was marked as outdated.
This comment was marked as outdated.
c6edf30 to
e038ae5
Compare
|
Hi, @ljharb, I've updated the code per your most recent feedback and also rebased. So now |
add CLI option --min=<version>, fallback to env var "NVM_MIN" if set. display a hint when version filtering is in effect Other minor changes: - fix: disable color in nvm_print_versions tests - trim leading space in mock output due to eslint errors - prefixed versions like v18 also work - remove function ref duplication - avoid inline initialization for ksh compatibility Co-authored-by: Jordan Harband <ljharb@gmail.com>

This is to fix #3250.