Skip to content

Issue4392 resolution - No new line after operation #4397

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

Closed
wants to merge 3 commits into from
Closed

Issue4392 resolution - No new line after operation #4397

wants to merge 3 commits into from

Conversation

davidBar-On
Copy link
Contributor

For issue #4392 resolution - not adding new line after operation ('==', etc.).
Also test cases file is added.

An issue in the code changes is that "transmute" is used. Hopefully there is better way to implement.

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.

Thank you for the PR @davidBar-On. However, this is not currently in a mergeable state, and is most likely just a compounding of an underlying formatting failure due to the comments.

Please again note that the CI checks are failing with your changes. You may want to take a look at the documentation on testing rustfmt again (links below), and please feel free to reach out with any questions here and/or on Discord

https://github.com/rust-lang/rustfmt/blob/master/Contributing.md#creating-test-cases
https://github.com/rust-lang/rustfmt/blob/master/Contributing.md#running-tests

Comment on lines +1607 to +1615
/* Trim white spaces at end of lines */
let mut c = String::from("");
for line in snippet.to_owned().split('\n') {
if c != "" {
c.push('\n')
};
c.push_str(line.trim_end());
}
Some(c.to_owned())
Copy link
Member

Choose a reason for hiding this comment

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

You've got these same changes in three separate PRs. Please keep various commits and changes separate for easier review and potential for merging.

As I noted in #4391 (comment), I do not believe this is the right approach to address the related issue.

};
let list_len = list.list.len();
/* ???? Is there better way than transmute() ???? */
let list_expr = unsafe { std::mem::transmute::<&T, &ast::Expr>(e) };
Copy link
Member

Choose a reason for hiding this comment

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

As noted on the other thread, there's a root problem with comments between the subexpr and type in cast expressions which is most certainly impacting the formatting you are seeing with your snippets.

We really need to focus on the addressing the core issue instead of associated items, especially an approach like this as there are currently no unsafe blocks within the rustfmt source to my knowledge.

@davidBar-On
Copy link
Contributor Author

OK, Thanks. I now see that the root issue may be related to the issues discussed in the other thread, so this issue should be retested after the other issues are solved.

Regarding the repeated changes this was unintentional and I will try to make sure that doesn't happen (unless the same change is part of solving different issues).

@davidBar-On davidBar-On deleted the issue4392 branch August 28, 2020 07:28
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