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

Fix issues with formatting imports with comments #5853

Merged
merged 3 commits into from
Aug 31, 2023

Conversation

rsammelson
Copy link
Contributor

Fixes #5852

let result = if (list_str.contains('\n') || list_str.len() > remaining_width)
let result = if (list_str.contains('\n')
|| list_str.len() > remaining_width
|| tactic == DefinitiveListTactic::Vertical)
Copy link
Contributor

Choose a reason for hiding this comment

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

When you get a chance can you explain why we need to add the tactic == DefinitiveListTactic::Vertical) check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to resolve the second problem shown in the issue. The problem is that it will try to put in on one line because the list doesn't contain a newline (because there is only one actual item, and the newlines are only between items).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. It looks like tactic is somewhat influenced by the user's specified value for imports_layout. Out of an abundance of caution can you make sure that this fix holds for all values of imports_layout. Probably best to create a issue-5852 directory and add a test case for each imports_layout variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ytmimi sorry for the delayed response, but it should be fixed now

tests/source/use.rs Outdated Show resolved Hide resolved
@rsammelson rsammelson force-pushed the fix-import branch 3 times, most recently from aae1ac3 to ef449a1 Compare July 28, 2023 02:01
@ytmimi
Copy link
Contributor

ytmimi commented Aug 3, 2023

@rsammelson when you have a moment can you quickly verify that this PR addresses #5877. I'm almost positive that it does I just want to get confirmation.

@rsammelson
Copy link
Contributor Author

Yes, that is what this fixes.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 3, 2023

Yes, that is what this fixes.

Thank you for the quick reply!

@rsammelson
Copy link
Contributor Author

What is the status on this being merged?

@calebcartwright
Copy link
Member

What is the status on this being merged?

Somewhat vague answer though it's a somewhat vague question, but essentially we'll get to it when we can. The code diff looks small and the tests look thorough which is a great sign (and thank you for all the tests!).

I've kicked off one of our automated jobs that does more thorough levels of idempotence testing to try to give us a greater degree of confidence the proposed changes won't introduce any unexpected/unintentional formatting side effects, let's see how that goes -> https://github.com/rust-lang/rustfmt/actions/runs/5849405920

@rsammelson
Copy link
Contributor Author

It looks like there might be a problem, so I'll take a look at it.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 25, 2023

@rsammelson rebasing on the latest master should resolve the issue in the diff check job.

The diff I'm seeing occurring because your branch doesn't include the fix for #5882

-macro_rules! foo {}
+macro_rules! foo {
+    
+}

@ytmimi
Copy link
Contributor

ytmimi commented Aug 25, 2023

Thanks for rebasing. Let's give the job another run.

Edit: Job ran successfully ✅

@ytmimi
Copy link
Contributor

ytmimi commented Aug 25, 2023

@rsammelson I believe we could close #5290 if we get this merged. When you have a moment, can you double check that this also resolves #4708.

I have a feeling that this doesn't quite resolve #3984, specifically this input might still be buggy:

use a::{item /* comment */};
use b::{
    a,
    // comment
    item,
};
use c::item /* comment */;
use d::item; // really long comment (with `use` exactly 100 characters) ____________________________

use std::e::{/* it's a comment! */ bar /* and another */};
use std::f::{/* it's a comment! */ bar};
use std::g::{bar /* and another */};

I'm not suggesting you need to solve this one here, but I do want to make a note of it.

@rsammelson
Copy link
Contributor Author

Yes, that issue looks the same as one of the test cases I have, so it should be fixed.

@rsammelson
Copy link
Contributor Author

use a::{item /* comment */};
use b::{
    a,
    // comment
    item,
};
use c::item /* comment */; // THIS LINE
use d::item; // really long comment (with `use` exactly 100 characters) ____________________________

use std::e::{/* it's a comment! */ bar /* and another */};
use std::f::{/* it's a comment! */ bar};
use std::g::{bar /* and another */};

@ytmimi Actually, it looks like the only thing broken in that block is the marked line, which is fixed? (I'm not sure how changes are incorporated here, since the fix isn't in master) in #4759. I added a test with the other lines.

@rsammelson rsammelson force-pushed the fix-import branch 2 times, most recently from 34c224e to 8641150 Compare August 26, 2023 07:10
@ytmimi
Copy link
Contributor

ytmimi commented Aug 26, 2023

@ytmimi Actually, it looks like the only thing broken in that block is the marked line, which is fixed? (I'm not sure how changes are incorporated here, since the fix isn't in master) in #4759. I added a test with the other lines.

@rsammelson thanks for looking into this! #4759 is a great find! The PR is marked as backport-triage, which means that the fix isn't currently available on the trunk branch. If you run git branch -a --contains e10d2d7 you'll see that the commit is available on the rustfmt-2.0.0-rc.2 branch.

I don't mean to keep expanding the scope of this PR, but would you mind investigating whether applying similar changes would resolve the issue?

@rsammelson
Copy link
Contributor Author

Alright, I will try to backport that fix and move the tests; if I can't backport it I'll just get rid of the test for now.

@rsammelson
Copy link
Contributor Author

I was able to pull in the commit, and it fixes the problem from the test block you mentioned, but the test cases added in the fix commit are still broken so the backport is not completely successful. I think it's sufficient for this PR though.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 26, 2023

I was able to pull in the commit, and it fixes the problem from the test block you mentioned, but the test cases added in the fix commit are still broken so the backport is not completely successful. I think it's sufficient for this PR though.

Just to be clear, what you're saying is that more work is needed to fully backport #4759?

Could you highlight which test cases are still broken?

@rsammelson
Copy link
Contributor Author

Yes, all test files added in that PR are broken (mostly incorrect indentation), but applying (at least part of) the fix doesn't break anything that wasn't already broken. It fixed the problem with use thing /* comment */;.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 28, 2023

Yes, all test files added in that PR are broken (mostly incorrect indentation), but applying (at least part of) the fix doesn't break anything that wasn't already broken. It fixed the problem with use thing /* comment */;.

If I remember correctly that's because those multi-line comment fixes are predicated on a different backport, but I agree that those changes aren't necessary to solve the current issue.

I think we're good to go here. My last ask is to squash your first two commits into one and rebase on the latest master. I think we should keep the backport commit and test cases for #3984, separate.

@rsammelson
Copy link
Contributor Author

Alright it should be good now.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 31, 2023

@rsammelson the backport commit technically needs to come before the Improve tests commit since the tests for #3984, will fail if the backport isn't applied. Can you adjust the order of the commits?

@rsammelson
Copy link
Contributor Author

Good point, I didn't think about that. It's fixed.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

I appreciate you sticking with this through all the revisions I've asked for.

I ran this diff check job before the commits were reordered and I ran this diff check job after the commits were reordered (just being overly cautious), and both passed so I'm feeling very good about this!

Oh, and thanks for your first contribution to rustfmt 🎉

@ytmimi ytmimi merged commit b636723 into rust-lang:master Aug 31, 2023
27 checks passed
@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-not-reviewed labels Aug 31, 2023
@rsammelson
Copy link
Contributor Author

Thanks!

@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Oct 23, 2023
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.

Comments in imports are formatted incorrectly
5 participants