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

Dropping of carriage returns #129

Closed
brettz9 opened this issue May 22, 2021 · 6 comments
Closed

Dropping of carriage returns #129

brettz9 opened this issue May 22, 2021 · 6 comments

Comments

@brettz9
Copy link
Contributor

brettz9 commented May 22, 2021

We had an issue reported at gajus/eslint-plugin-jsdoc#745 regarding carriage returns being dropped.

It appears that in the splitLines function, a splitis taking place which may consume the carriage return, but joins are not adding back what was found to be there.

Would it work and be enough to just drop the matching of \r? in the split regex (or make its consumption optional), letting consumers deal with stripping out any unwanted carriage returns?

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 14, 2021

Hi there... Do you think you may have some time to take a look at this?

@syavorsky
Copy link
Owner

Yes, this is in my TODO list. Unfortunately, I am blocked badly with work these days. Will try my best to cut some time for it

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 14, 2021

Was just bumping in case it fell off the radar. Please don't feel any pressure.

@syavorsky
Copy link
Owner

@brettz9 please check out the comment-parser@1.1.6-beta.0. Let me know if I got it right. It seems to be a safe change overall but I am going to add more tests

syavorsky added a commit that referenced this issue Jul 6, 2021
syavorsky added a commit that referenced this issue Jul 7, 2021
syavorsky added a commit that referenced this issue Jul 26, 2021
@syavorsky
Copy link
Owner

Fixed in 1.2.0. Carriage returns are split into separate token lineEnd and kept untouched until eventually merged back with stringify()

@brettz9
Copy link
Contributor Author

brettz9 commented Jul 26, 2021

Great...this new approach was a good way to go, I think. Thanks!

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

No branches or pull requests

2 participants