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 Bug: RemoveAndSortUsings #727

Merged

Conversation

kyleruddbiz
Copy link
Contributor

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.

  1. Ensure CodeMaid Options match included image.
    image
  2. Cleanup Active Document.

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

Prevent duplicate using statements from being re-added.
Copy link
Owner

@codecadwallader codecadwallader left a 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");
Copy link
Owner

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.

Copy link
Contributor Author

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();
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
ThreadHelper.ThrowIfNotOnUIThread();

Although the IDE provides warnings about this, in general I've found we can invoke on background threads and boost performance.

Copy link
Contributor Author

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
Copy link
Owner

@codecadwallader codecadwallader Jun 7, 2020

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. 👍

Copy link
Contributor Author

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.

@kyleruddbiz
Copy link
Contributor Author

Thanks for getting back to me. I made changes based on your comments. Let me know if you need any other adjustments.

@codecadwallader codecadwallader added this to the v11.2 milestone Sep 22, 2020
@codecadwallader codecadwallader merged commit a31b08a into codecadwallader:master Sep 22, 2020
@codecadwallader
Copy link
Owner

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

@kyleruddbiz
Copy link
Contributor Author

Happy to help, I look forward to the next update!

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.

None yet

2 participants