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] Improve .nvmrc reading process #1740

Merged
merged 1 commit into from
Apr 15, 2018

Conversation

PeterDaveHello
Copy link
Collaborator

@PeterDaveHello PeterDaveHello commented Feb 20, 2018

Improve how we read the version in .nvmrc to avoid the impact of CR/CRLF as newline char.

Fixes #1015. Fixes #1712.

nvm.sh Outdated
@@ -293,7 +293,7 @@ nvm_rc_version() {
nvm_err "No .nvmrc file found"
return 1
fi
read -r NVM_RC_VERSION < "${NVMRC_PATH}" || printf ''
NVM_RC_VERSION="$(command head -n 1 "${NVMRC_PATH}" | command tr -d '\r')" || printf ''
Copy link
Member

Choose a reason for hiding this comment

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

This will need tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@ljharb ljharb changed the title [Fix] Improve .nvmrc reading process, fix #1015, fix #1712 [Fix] Improve .nvmrc reading process Feb 21, 2018
@PeterDaveHello PeterDaveHello force-pushed the handle-cr-char-in-nvmrc branch 2 times, most recently from bba70f5 to 7984305 Compare February 21, 2018 10:49
\. ../../nvm.sh

# normal .nvmrc
printf '0.999.0\n' > .nvmrc
Copy link
Member

Choose a reason for hiding this comment

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

should we also add an example that has no newline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

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.

This looks good, but it's a pretty important part of the code to change, so I'm going to let it bake for a bit before merging, and test it locally.

@PeterDaveHello
Copy link
Collaborator Author

Great, let's hope to get it done soon!

@MystK
Copy link

MystK commented Apr 1, 2018

I tested this out with both linux new lines and windows new lines and it seemed to have worked!

@PeterDaveHello
Copy link
Collaborator Author

I hope > 1.5 months bake is good enough now :)

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.

3 participants