Skip to content
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

[Fix] Fix sed syntax error in nvm_command_info() #1528

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

PeterDaveHello
Copy link
Collaborator

No description provided.

nvm.sh Outdated
@@ -54,7 +54,7 @@ nvm_command_info() {
local INFO
COMMAND="${1}"
if type "${COMMAND}" | command grep -q hashed; then
INFO="$(type "${COMMAND}" | command sed -E 's/\(|)//g' | command awk '{print $4}')"
INFO="$(type "${COMMAND}" | command sed -E 's/\(|\)//g' | command awk '{print $4}')"
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on this one? it doesn't seem to error out the sed call when I try it locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The origin one will cause error on GNU sed:
sed: -e expression #1, char 9: Unmatched ) or \)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I've reproduced it - do none of the current travis tests trigger this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we have test for that yet, won't mind that if you can directly add it.

@PeterDaveHello
Copy link
Collaborator Author

@ljharb is there anything else I can do here? Thanks.

@ljharb
Copy link
Member

ljharb commented Jun 24, 2017

I'd like to have a regression test before merging this.

@PeterDaveHello
Copy link
Collaborator Author

Any tip please?

@ljharb
Copy link
Member

ljharb commented Jun 24, 2017

It seems like a unit test for nvm_command_info would trigger that line of code; even if travis-ci didn't trigger the failure, it'd still be good to have the regression test.

@PeterDaveHello
Copy link
Collaborator Author

Is there any regression test example exist in nvm right now so that I can take a look how should it looks like here?

@ljharb
Copy link
Member

ljharb commented Jun 24, 2017

Just all the existing unit tests - what makes a test a "regression test" is merely that the test was added in response to a bug; to prevent a bug fix from "regressing" in the future.

@PeterDaveHello
Copy link
Collaborator Author

Oh, great, I misunderstood, thanks, just thinking about how the test would look like 🤔

@PeterDaveHello
Copy link
Collaborator Author

@ljharb I'm sorry that I can't make it recently, with all due respect, as it's been a while, and we do locate the bug, know how to fix it, just lack of the test, I wonder if we can

  1. Just get this merged, and I'll open another issue to mention this missing test.
    or
  2. You'd like to help directly take care of the testing part and get this merged?

It'll be great if we can get this bug fixed and landed in the recent release, thanks a lot.

@PeterDaveHello
Copy link
Collaborator Author

Just simply detect any standard error message in the test, as nvm_command_info should not have a standard error output, what do you think :)

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 like this will work!

@ljharb ljharb merged commit d5dacdf into nvm-sh:master Apr 23, 2018
@PeterDaveHello PeterDaveHello deleted the fix-nvm-debug branch April 24, 2018 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants