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 newlines in JSON output #4262

Merged
merged 2 commits into from
Jun 23, 2020
Merged

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Jun 19, 2020

This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line.

When feeding the results into arc lint this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with \r\n endings (though that would have been the case before too).

Fixes #4259

This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line.

When feeding the results into `arc lint` this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with `\r\n` endings (though that would have been the case before too).

Fixes rust-lang#4259
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Timmmm!

I have only done limited testing though, in particular there's a possibility it might not work with files with \r\n endings (though that would have been the case before too).

Would like to do some additional testing with this as I mentioned inline, mostly to make sure the line numbers still match up in larger files with a lot of shifting around.

You're spot on about the newline style potentially being off, though that's already the case (no change/impact with this PR) and could be addressed in a separate follow up PR.

It might be useful to add some unit tests for a few additional scenarios that I don't think are covered in the existing tests (if you're up for it!), for example where rustfmt would remove blank lines, insert blank lines, and some combinations of both

@calebcartwright
Copy link
Member

Fantastic, thanks so much! I'm going to go ahead and merge this given the improvement, and will open a separate issue for the newline style

@calebcartwright calebcartwright merged commit e208a39 into rust-lang:master Jun 23, 2020
karyon pushed a commit to karyon/rustfmt that referenced this pull request Oct 28, 2021
* Fix newlines in JSON output

This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line.

When feeding the results into `arc lint` this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with `\r\n` endings (though that would have been the case before too).

Fixes rust-lang#4259

* Update tests
# Conflicts:
#	tests/writemode/target/output.json
calebcartwright pushed a commit that referenced this pull request Jan 2, 2022
* Fix newlines in JSON output

This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line.

When feeding the results into `arc lint` this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with `\r\n` endings (though that would have been the case before too).

Fixes #4259

* Update tests
# Conflicts:
#	tests/writemode/target/output.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON output for removing blank lines is wrong
3 participants