-
Notifications
You must be signed in to change notification settings - Fork 985
Do not merge top-level modules with imports_granularity Module #6784
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
Do not merge top-level modules with imports_granularity Module #6784
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the example from #6191 as a test case:
Specifically can you show that the following:
use {library1, library2 as lib2, library3};gets rewritten as follows since we're not merging top level imports:
use library1;
use library2 as lib2;
use library3;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what my addition to the test files does. But ok, I can make it match verbatim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your test case shows that they're not getting merged, which I think is good, but what I want to demonstrate is that top level modules that are already merged will get unmerged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not suggesting you remove what you've already got, just add the example from the original issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah makes sense, added it now.
| use a::{f, g::{h, i}}; | ||
| use a::{j::{self, k::{self, l}, m}, n::{o::p, q}}; | ||
| use a::{ | ||
| f, | ||
| g::{h, i}, | ||
| }; | ||
| use a::{ | ||
| j::{ | ||
| self, | ||
| k::{self, l}, | ||
| m, | ||
| }, | ||
| n::{o::p, q}, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, what's going on here? Why did all this change now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of these changes are necessary. Let's revert all changes to tests/source/imports/imports_granularity_module.rs that aren't related to adding use {library1, library2 as lib2, library3};.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh whoa, that was not intended. It must have been my vscode messing up the formatting. Will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now.
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
Signed-off-by: mwlon <m.w.loncaric@gmail.com>
Signed-off-by: mwlon <m.w.loncaric@gmail.com>
Signed-off-by: mwlon <m.w.loncaric@gmail.com>
This reverts commit 820451b.
Signed-off-by: mwlon <m.w.loncaric@gmail.com>
cf4b73d to
f89b67c
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Thanks for fixing the tests |
Fixes #6191 .