Skip to content

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

@PeterDaveHello PeterDaveHello force-pushed the handle-cr-char-in-nvmrc branch from 7984305 to 787bbe3 Compare February 25, 2018 08:20
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