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

Correct newline style in json emitter #4743

Open
wants to merge 1 commit into
base: rustfmt-2.0.0-rc.2
Choose a base branch
from

Conversation

davidBar-On
Copy link
Contributor

Suggested fix for issue #4273. The main change is in add_misformatted_file() to use the proper newline styles in each of the original/expected fields in each MismatchedBlock. The original code"s newline style is used for original while the config newline_style is used for expected.

The other changes are to add parameters to functions/structures to allow passing the config newline_style value to add_misformatted_file(). The new parameters required adding them to function calls in some of the emitter/json.rs test cases. In all these cases default values used.

Test new cases were added to emitter/json.rs, assuming there is no way to verify the json output using test cases files.

The approach taken is that add_misformatted_file() gets the proper newline strings and continues to add the newline to the end of the original/expected strings, using these values instead of constant \n. A simpler alternative may have been to continue adding \n to the end of the strings and then call apply_newline_style() to convert them to the proper newline style. However, that approach seem to be problematic from performance point of view, as the call is done separately for each original/expected field for each MismatchedBlock.

(This is probably the last new PR I am submitting in a while. I know it is a lot of effort and time to review and verify the changes, but I had the time and energy lately to try resolving some of the open issues. Still, if there are issues I can help resolving I will be happy to do that.)

@davidBar-On davidBar-On force-pushed the issue-4273-newline-style-in-json-emitter branch from 6941af0 to 5ec833b Compare March 8, 2021 10:01
@calebcartwright
Copy link
Member

(This is probably the last new PR I am submitting in a while. I know it is a lot of effort and time to review and verify the changes, but I had the time and energy lately to try resolving some of the open issues. Still, if there are issues I can help resolving I will be happy to do that.)

Thanks for letting me know, and thanks for all your contributions! Will you still be available to address any review items on your open PRs and help get them over the finish line?

@davidBar-On
Copy link
Contributor Author

Will you still be available to address any review items on your open PRs and help get them over the finish line?

Yes, I have the time. Actually, as I wrote, if there are issues you think I may be able to help with I will be happy to do that.

There are two reasons to what I wrote: one is that I didn"t find other help wanted issues that I thought I can help with. The other reason is that I fill that lately I may have overloaded you with new PRs so it seems I should take a break (I see that you do large part of the reviews on weekends).

@calebcartwright calebcartwright changed the base branch from master to rustfmt-2.0.0-rc.2 April 30, 2021 00:27
@calebcartwright
Copy link
Member

Test new cases were added to emitter/json.rs, assuming there is no way to verify the json output using test cases files.

Still haven"t gotten to this one yet, but note it is possible to use input source files and json output files. Some examples for emit modes can be found in https://github.com/rust-lang/rustfmt/tree/master/tests/writemode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-low pr-not-reviewed pr-targeting-2.0 This PR is targeting the 2.0 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants