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

🐛 Bug: Dedent not working with carriage returns \r #94

Closed
3 tasks done
matthewt-assurity opened this issue Aug 7, 2024 · 3 comments
Closed
3 tasks done

🐛 Bug: Dedent not working with carriage returns \r #94

matthewt-assurity opened this issue Aug 7, 2024 · 3 comments
Labels
status: in discussion Not yet ready for implementation or a pull request type: bug Something isn't working :(

Comments

@matthewt-assurity
Copy link

matthewt-assurity commented Aug 7, 2024

Bug Report Checklist

  • I have tried restarting my IDE and the issue persists.
  • I have pulled the latest main branch of the repository.
  • I have searched for related issues and found none that matched my issue.

Expected

I've got code that splits a string by newlines and carriage returns into lines. I've got a test that checks that newlines and carriage returns have been removed.

const myString = dedent`
            line one\r
            line two
            line three`

const lines = myString.split(/\r\n|\r|\n/)

When console.logging lines, I would expect to see the below:

[
  'line one',
  'line two',
  'line three'
]

Actual

What I actually see when I console.log lines is this. It appears the \r is not removed, and \\r is added in.

[
  'line one\\r',
  'line two',
  'line three'
]

Additional Info

I can get my use-case working by not using dedent, so I'm not blocked, but I sure would like to use dedent to make my strings look nicer!

const myString = `line one\r
line two
line three`
@matthewt-assurity matthewt-assurity added the type: bug Something isn't working :( label Aug 7, 2024
@matthewt-assurity matthewt-assurity changed the title 🐛 Bug: <short description of the bug> 🐛 Bug: Dedent not working with carriage returns \r Aug 7, 2024
@JoshuaKGoldberg
Copy link
Collaborator

JoshuaKGoldberg commented Aug 8, 2024

Ha, interesting issue - thanks for filing! I always forget \r sometimes appears on its own...

But: I'm not sure it's clear that a \r on its own should be trimmed? Reading https://stackoverflow.com/questions/1761051/difference-between-n-and-r:

in Unix and all Unix-like systems, \n is the code for end-of-line, \r means nothing special

Is there a reason strings would have lone \r without \n?

@JoshuaKGoldberg JoshuaKGoldberg added the status: in discussion Not yet ready for implementation or a pull request label Aug 8, 2024
@matthewt-assurity
Copy link
Author

I've seen the issue when I try to split the result from a FileReader.

Given I have a file (e.g from an HTML input element) I can run the below to log the split lines of the file to the console:

const reader = new FileReader();

reader.onload = () => {
	const lines = reader.result.split('\n');
	console.log(lines);
};

reader.readAsText(file);

This results in the output containing \r at the end of each line.

However, based on what you've said I might be handling this incorrectly... should I always be trying to split with the full /\r\n|\r|\n/?

Bringing this back to dedent, I am using dedent to write some test data strings for my test functions to use (so I don't have to parse a file for testing purposes, mainly because it seems the JS FileReader API can't be used in NodeJS :/ ). I wanted to test for cases where strings end with \r (re: my original post), but is it possible I just have the wrong understanding and this is a non-issue? Am I handling things incorrectly by splitting only by \n?

@JoshuaKGoldberg
Copy link
Collaborator

should I always be trying to split with the full /\r\n|\r|\n/?

Yes. If it's possible that your system uses \r\n to delineate a newline, then it's on the splitting code to respect that.

Thanks for filing!

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in discussion Not yet ready for implementation or a pull request type: bug Something isn't working :(
Projects
None yet
Development

No branches or pull requests

2 participants