Skip to content

Fix #2728. #2785

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

Merged
merged 1 commit into from
Jun 19, 2018
Merged

Fix #2728. #2785

merged 1 commit into from
Jun 19, 2018

Conversation

wada314
Copy link
Contributor

@wada314 wada314 commented Jun 13, 2018

Do you mind if I clone() the whole config object?

src/lib.rs Outdated
@@ -718,10 +718,16 @@ fn format_code_block(code_snippet: &str, config: &Config) -> Option<String> {
let mut result = String::with_capacity(snippet.len());
let mut is_first = true;

// While formatting the code, ignore the config's newline style setting and always use "\n"
// instead of "\r\n" for the newline characters.
Copy link
Member

Choose a reason for hiding this comment

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

Why is it OK to do this? It seems like this would insert the wrong line ending if the user wants Windows line endings.

@wada314
Copy link
Contributor Author

wada314 commented Jun 18, 2018

We are doing '\n' ==> "\r\n' conversion for the whole output files in another place, so it should be okay. All tests are still passing in Windows after this fix.
This place is doing in-comment code block formatting. The output here is not rustfmt final output but an input to comment.rs. We are expecting '\n' newline here.

@nrc
Copy link
Member

nrc commented Jun 18, 2018

Could you add that information to a comment please? r+ with that

@wada314
Copy link
Contributor Author

wada314 commented Jun 19, 2018

Added comment. The automatic test is failing but it looks it's not related to my PR.

@nrc nrc merged commit fb55135 into rust-lang:master Jun 19, 2018
@nrc
Copy link
Member

nrc commented Jun 19, 2018

Thanks!

@wada314 wada314 deleted the issue-2728 branch December 25, 2018 22:57
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

Successfully merging this pull request may close these issues.

2 participants