Skip to content

ls-remote accepts versions ending with a dot #2310

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.

Sign up for GitHub

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

Jump to bottom
Merged
ljharb merged 1 commit into from
Oct 31, 2020
Merged

ls-remote accepts versions ending with a dot #2310

ljharb merged 1 commit into from
Oct 31, 2020

Conversation

@gitburd
Copy link
Contributor

@gitburd gitburd commented Sep 24, 2020 •
edited
Loading

Issue:

nvm ls-remote will not filter versions ending with "."
Resolves issue #983.

Example:

$ nvm ls-remote 5
->       v5.0.0
         v5.1.0
         v5.1.1
         v5.2.0
         v5.3.0
         v5.4.0
         v5.4.1
         v5.5.0
         v5.6.0
         v5.7.0
         v5.7.1
         v5.8.0
         v5.9.0
         v5.9.1
        v5.10.0
        v5.10.1
        v5.11.0
        v5.11.1
        v5.12.0

$ nvm ls-remote 5.
            N/A

After change:

$ nvm ls-remote 5.
->       v5.0.0
         v5.1.0
         v5.1.1
         v5.2.0
         v5.3.0
         v5.4.0
         v5.4.1
         v5.5.0
         v5.6.0
         v5.7.0
         v5.7.1
         v5.8.0
         v5.9.0
         v5.9.1
        v5.10.0
        v5.10.1
        v5.11.0
        v5.11.1
        v5.12.0

@gitburd gitburd force-pushed the ls-remote-filter-dot branch from 259679b to 3f333db Compare September 25, 2020 00:49
ljharb
ljharb requested changes Sep 26, 2020
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems great! Can we add a regression test? :-)

@gitburd gitburd force-pushed the ls-remote-filter-dot branch 2 times, most recently from 2531c6b to bebb770 Compare September 28, 2020 17:11
@gitburd
Copy link
Contributor Author

gitburd commented Sep 29, 2020

I would love to add a regression test, what is that?

@ljharb
Copy link
Member

ljharb commented Sep 29, 2020

In this case, in the same way that test/fast/Unit\ tests/nvm\ ls-remote mocks out nvm_ls_remote, a unit test could be added, of nvm_ls_remote, that mocks out nvm_ls_remote_index_tab (with the mock data) in order to exercise this logic.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2020

To clarify; a "regression test" is a test that would have failed without the PR's fix, but now passes - this prevents a regression, or recurrence, of the bug.

@ljharb
Copy link
Member

ljharb commented Oct 14, 2020

@gitburd is there anything i can help unblock for you on writing a test for this PR? I'd love to get this fix into the next release.

@gitburd gitburd force-pushed the ls-remote-filter-dot branch from bebb770 to fb89405 Compare October 29, 2020 18:10
@gitburd
Copy link
Contributor Author

gitburd commented Oct 29, 2020

Sorry for the delay @ljharb I was blocked by being very ill but I can finally start working again. I added a test to test/fast/Unit tests/nvm_ls_remote, could this work?

@gitburd gitburd force-pushed the ls-remote-filter-dot branch 2 times, most recently from 6ec1c1c to 44cf2f5 Compare October 29, 2020 18:34
ljharb
ljharb approved these changes Oct 31, 2020
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Glad you're recovered! This looks great.

@ljharb ljharb force-pushed the ls-remote-filter-dot branch from 44cf2f5 to c72f2c6 Compare October 31, 2020 05:49
@ljharb ljharb merged commit c72f2c6 into nvm-sh:master Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gitburd @ljharb