Skip to content

Fix regex pattern in _nvm_list to correctly match version directory names #227

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

Merged

Conversation

NV4RE
Copy link
Contributor

@NV4RE NV4RE commented Sep 8, 2024

Description:

This PR fixes an issue with the _nvm_list function where the regular expression pattern "v\d.+" was too broad and could match usernames that contain a v followed by digits.

The updated code changes the regular expression pattern to "/v\d.+" to match version directory names with a leading slash, and then removes the leading slash from the matched version directory names using string replace.

This fix ensures that the _nvm_list function correctly matches version directory names and produces the expected output.

Changes:

  • Added a new local variable installed_versions to store the version directory names without the path
  • Used string replace to remove the $nvm_data/ prefix from each version directory name
  • Updated the regular expression pattern to match only the version directory names

@jorgebucaran
Copy link
Owner

Extremely helpful and quick turnaround. I'll review this tomorrow, but it looks pretty good. Should we maybe first replace and then match, or am I misreading it?

@NV4RE
Copy link
Contributor Author

NV4RE commented Sep 9, 2024

Extremely helpful and quick turnaround. I'll review this tomorrow, but it looks pretty good. Should we maybe first replace and then match, or am I misreading it?

Updated:

Changes:

  • Added a new local variable installed_versions to store the version directory names without the path
  • Used string replace to remove the $nvm_data/ prefix from each version directory name
  • Updated the regular expression pattern to match only the version directory names

Copy link
Owner

@jorgebucaran jorgebucaran left a comment

Choose a reason for hiding this comment

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

We should be able to simplify this to:

set --local versions (string replace --all -- $nvm_data/ "" $nvm_data/*)

Could you confirm that works for you and update your commit? I'll merge it once that's done.

@jorgebucaran jorgebucaran added the enhancement New feature or fix label Sep 9, 2024
@NV4RE
Copy link
Contributor Author

NV4RE commented Sep 9, 2024

We should be able to simplify this to:

set --local versions (string replace --all -- $nvm_data/ "" $nvm_data/*)

Could you confirm that works for you and update your commit? I'll merge it once that's done.

I confirm that simplified change work correctly on my machine

Copy link
Owner

@jorgebucaran jorgebucaran left a comment

Choose a reason for hiding this comment

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

Let's do this! 👍

@jorgebucaran jorgebucaran merged commit 46586c4 into jorgebucaran:main Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants