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

Improved implementation of VerifyFixAsync that will ignore diagnostics… #935

Merged
merged 4 commits into from
Jul 12, 2015

Conversation

vweijsters
Copy link
Contributor

… for which code fixes are not available.

@vweijsters
Copy link
Contributor Author

Running this will result in 16 failed tests. See also #936

@sharwell
Copy link
Member

The behavior of this change does not match the behavior of the IDE for the same code fixes. While the code fix creates separate XmlTextSyntax nodes for the newline and the following summary text, the IDE merges these two into a single XmlTextSyntax with two tokens (XmlTextLiteralNewLineToken and XmlTextLiteralToken) before invoking the analyzers again.

@vweijsters
Copy link
Contributor Author

I implemented a workaround for #936. It is my opinion that there was nothing wrong with the code to begin with. The fact that the IDE merges two tokens is an implementation detail of Roslyn and our code should not depend on that.

I added the workaround to removing a blocking factor for this PR. Given the complexity of the code affected by #936, it seemed a better solution to me to add a workaround for now.

@sharwell
Copy link
Member

I added the workaround to removing a blocking factor for this PR.

At first glance, the workaround is exactly what I had in mind. 😄

It is my opinion that there was nothing wrong with the code to begin with.

💭 In an ideal world, the analyzers would treat equivalent representations of the source code in an equivalent manner. I'm comfortable knowing that we have regression tests in place to alert us to any change in the representation of the comments provided by Roslyn that would impact the behavior of the analyzers.

sharwell and others added 2 commits July 7, 2015 22:03
This commit removes an explicit text comparison in favor of a duplicate
diagnostics detection which was already being computed anyway. It also
uses Document.WithText instead of constructing a whole new Document
instance.
Tweak changed document detection
@sharwell
Copy link
Member

That was a big improvement. Commit e0304de took 8:17 minutes on AppVeyor, but the updated commit bb9cee1 to only 6:50 minutes. It's still slower than prior to this pull request but not as much slower.

Hopefully performance improvements to xunit can improve this more in the future.

sharwell added a commit that referenced this pull request Jul 12, 2015
Improved implementation of VerifyFixAsync that will ignore diagnostics…
@sharwell sharwell merged commit 408e0d2 into DotNetAnalyzers:master Jul 12, 2015
@sharwell sharwell added this to the 1.0.0 Alpha 11 milestone Jul 12, 2015
@sharwell sharwell self-assigned this Jul 12, 2015
@vweijsters vweijsters deleted the fix-codefixverifier branch July 12, 2015 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants