Skip to content

strip \r (if present) before final \n #4329

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
merged 1 commit into from
Oct 4, 2016
Merged

strip \r (if present) before final \n #4329

merged 1 commit into from
Oct 4, 2016

Conversation

shreeve
Copy link
Contributor

@shreeve shreeve commented Oct 2, 2016

Fixes 'cake test', which was failing due to "\r\n" terminators

@lydell
Copy link
Collaborator

lydell commented Oct 3, 2016

This sounds like #4319. Could you please look through the comments over there, and then say if this PR is still relevant or not?

@shreeve
Copy link
Contributor Author

shreeve commented Oct 3, 2016

Thanks for the reference to #4319, and yes this is the same issue. It appears to affect Mac OS only.

This PR has a better fix than the one from #4319 and should safely close both issues.

@lydell
Copy link
Collaborator

lydell commented Oct 3, 2016

So the Node.js version doesn’t matter for you?

@shreeve
Copy link
Contributor Author

shreeve commented Oct 3, 2016

I just upgraded to node 6.7.0 and this PR is not required.

Perhaps it was a transient thing that only impacted node versions on Mac between 6.4.x and 6.6.x?

Having said that, this small fix might help people on those versions and shouldn't hurt anything otherwise. It just just does this:

-    @written[@written.length - 1 + fromEnd].replace /\n$/, ''
+    @written[@written.length - 1 + fromEnd].replace /\r?\n$/, ''

@lydell lydell merged commit 11561dd into jashkenas:master Oct 4, 2016
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