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.

Sign up for GitHub

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

Jump to bottom

Fixed 'Incorrect comment indent inside if/else' issue. #4459

Merged
merged 8 commits into from
Oct 15, 2020
Merged

Fixed 'Incorrect comment indent inside if/else' issue. #4459

merged 8 commits into from
Oct 15, 2020

Conversation

whizsid
Copy link
Contributor

@whizsid whizsid commented Oct 7, 2020

Fixes #4120 .

@calebcartwright
Copy link
Member

Could you elaborate on the removal of the existing test file?

@whizsid
Copy link
Contributor Author

whizsid commented Oct 8, 2020

@calebcartwright It was implemented to test #1575 and there are conflicts between the two cases. And already covered these cases in the new test file.

And no, this definitely should not be a comment on the else — actually,
it should not be rustfmt's decision whether this applies to the else or is a free comment after the 1.

Should we need to detect the user's indentation and format the last comment with the user's indentation?

@whizsid
Copy link
Contributor Author

whizsid commented Oct 10, 2020

@calebcartwright I changed the PR as you suggested in the comment #4120 (comment) . CI is failing because of a network error.

@calebcartwright
Copy link
Member

Excellent news @whizsid. I've restarted the GH run as it did indeed look spurious. I didn't get a chance to go through the implementation changes, but did a quick run through the tests added and they look great!

Will try to go through this in more detail tomorrow

topecongiro
topecongiro approved these changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@topecongiro topecongiro 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! LGTM.

calebcartwright
calebcartwright approved these changes Oct 15, 2020
View reviewed changes
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.

Well done @whizsid, thank you!

@calebcartwright calebcartwright merged commit 4834547 into rust-lang:master Oct 15, 2020
@calebcartwright calebcartwright mentioned this pull request Oct 15, 2020
Closed
calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this pull request Nov 14, 2020
@whizsid @calebcartwright
Fixed 'Incorrect comment indent inside if/else' issue. (rust-lang#4459)
b8f08f8 
* Added test cases

* Fixed if condition comment issue

* Fixed extern C issue

* Removed previous test case

* Removed tmp file

* honor the authors intent

* Changed the file name to its original name

* Removed extra whitespace
calebcartwright pushed a commit that referenced this pull request Nov 14, 2020
@whizsid @calebcartwright
Fixed 'Incorrect comment indent inside if/else' issue. (#4459)
faf97a6 
* Added test cases

* Fixed if condition comment issue

* Fixed extern C issue

* Removed previous test case

* Removed tmp file

* honor the authors intent

* Changed the file name to its original name

* Removed extra whitespace
@karyon
Copy link
Contributor

karyon commented Oct 26, 2021

Backported in #4525

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@topecongiro topecongiro topecongiro approved these changes

@calebcartwright calebcartwright calebcartwright approved these changes

Assignees
No one assigned
Labels
1x-backport:completed hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect comment indent inside if/else
4 participants
@whizsid @calebcartwright @karyon @topecongiro