-
Notifications
You must be signed in to change notification settings - Fork 364
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 Bug: RemoveAndSortUsings #727
Fix Bug: RemoveAndSortUsings #727
Conversation
Prevent duplicate using statements from being re-added.
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.
Thanks for putting this together! I have a couple comments about optimizations if you wouldn't mind taking a look when you have a chance please. :)
} | ||
|
||
// Now sort without removing to ensure correct ordering. | ||
_commandHelper.ExecuteCommand(textDocument, "Edit.SortUsings"); |
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.
This will cause the usings to be sorted twice for newer versions of the IDE, even for the default case where the using statements to re-insert do not exist. I can see why it is split out to happen after re-insertion, but I think it should be guarded to avoid extra work.
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.
Good catch. I'll update code to skip that work if there are no reinsert candidates.
.Where(usingStatement => TextDocumentHelper.FirstOrDefaultMatch(textDocument, string.Format(patternFormat, usingStatement)) == null) | ||
.ToList(); | ||
|
||
ThreadHelper.ThrowIfNotOnUIThread(); |
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.
ThreadHelper.ThrowIfNotOnUIThread(); |
Although the IDE provides warnings about this, in general I've found we can invoke on background threads and boost performance.
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, that makes sense. I'll remove it.
{ | ||
point.editPoint.CharRight(); | ||
} | ||
var usingStatementsToReinsert = _usingStatementsToReinsertWhenRemoved.Value |
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.
Previously I was trying to capture and re-insert text back where it was originally located. This helped with some custom sort rules that users had. I agree it may not be necessary anymore since newer versions of VS have merged remove and sort back together vs. being separate options. Thanks for simplifying the code. 👍
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 makes sense, no problem.
Thanks for getting back to me. I made changes based on your comments. Let me know if you need any other adjustments. |
Thanks again for putting this together and apologies for my delay in accepting it. This looks great and ran well on my machine so I'm glad to merge it! :) This bit of functionality has historically been a bit rough so I'm particularly excited to have it improved. <3 |
Happy to help, I look forward to the next update! |
Prevent duplicate using statements from being re-added.
This is somewhat related to #659, but I didn't see a bug describing this exact situation.
I'm including repro steps. Let me know if you would like me to move that out to a separate issue.
Visual Studio 2019 express. Windows 10.
Expected: Includes all referenced usings, and at most one instance of each using in CodeMaid options.
Actual: Reorders usings and adds duplicates.
RemoveAndSortUsings Bug Example.zip