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.

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

[refactor] use "case" instead of if/else in install #898

Merged
merged 1 commit into from
Jul 2, 2016
Merged

[refactor] use "case" instead of if/else in install #898

merged 1 commit into from
Jul 2, 2016

Conversation

PeterDaveHello
Copy link
Collaborator

Use case instead of multiple if/else statement.

@PeterDaveHello PeterDaveHello deleted the patch-1 branch November 9, 2015 15:24
@PeterDaveHello PeterDaveHello restored the patch-1 branch November 9, 2015 15:26
@PeterDaveHello PeterDaveHello reopened this Nov 9, 2015
@PeterDaveHello
Copy link
Collaborator Author

hmmmm ... failed on "SHELL=zsh TEST_SUITE=slow" ...

@ljharb ljharb changed the title Update nvm.sh [refactor] use "case" instead of if/else in install Dec 22, 2015
@ljharb ljharb added installing node Issues with installing node/io.js versions. needs followup We need some info or action from whoever filed this issue/PR. needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. labels Dec 22, 2015
@ljharb
Copy link
Member

ljharb commented Jun 26, 2016

@PeterDaveHello do you have any interest in completing this PR?

@PeterDaveHello
Copy link
Collaborator Author

Sure, give me few days, let's finish this in July.

@PeterDaveHello
Copy link
Collaborator Author

@ljharb Let's move on!

@ljharb ljharb removed the needs followup We need some info or action from whoever filed this issue/PR. label Jul 2, 2016
@ljharb
Copy link
Member

ljharb commented Jul 2, 2016

Thanks! After #1139 is rebased and merged, please rebase this one, and I'll merge it too :-)

@PeterDaveHello
Copy link
Collaborator Author

Rebased, but do we really need to do the rebase so frequently, almost do it before every merge?

@ljharb
Copy link
Member

ljharb commented Jul 2, 2016

We don't need to, but I really prefer the cleaner git log that doing so provides. If it's problematic for you, then I certainly can still merge it.

@PeterDaveHello
Copy link
Collaborator Author

Nope, not a really problem, and I think the squash and merge button will do that for us :)

@PeterDaveHello
Copy link
Collaborator Author

Is this good to go now?

@ljharb
Copy link
Member

ljharb commented Jul 2, 2016

Squash-and-merge leaves the PR commit(s) forever dangling on the commit graph (i have PRs set up to auto fetch) - sadly that feature from github was not useful for me.

Yes, I can just merge it as-is if you like.

@ljharb ljharb merged commit 1bcd2f4 into nvm-sh:master Jul 2, 2016
@PeterDaveHello PeterDaveHello deleted the patch-1 branch July 3, 2016 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
installing node Issues with installing node/io.js versions. needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PeterDaveHello @ljharb