-
Notifications
You must be signed in to change notification settings - Fork 909
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
Conversation
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
There was a problem hiding this 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
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 |
* 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
* 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
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