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.

Sign up for GitHub

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

Jump to bottom

Implement a custom fix all provider for SA1133 #1893

Merged
merged 1 commit into from
Dec 4, 2015
Merged

Implement a custom fix all provider for SA1133 #1893

merged 1 commit into from
Dec 4, 2015

Conversation

pdelvo
Copy link
Member

@pdelvo pdelvo commented Dec 3, 2015

Fixes #1879.

@codecov-io
Copy link

Current coverage is 79.44%

Merging #1893 into master will increase coverage by +0.04% as of f248ea1

@@            master   #1893   diff @@
======================================
  Files          558     558       
  Stmts        34824   34857    +33
  Branches      9645    9653     +8
  Methods          0               
======================================
+ Hit          27653   27693    +40
- Partial       5630    5637     +7
+ Missed        1541    1527    -14

Review entire Coverage Diff as of f248ea1

Powered by Codecov. Updated on successful CI builds.

vweijsters
vweijsters reviewed Dec 4, 2015
View reviewed changes
StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/ReadabilityRules/SA1133CodeFixProvider.cs
var indentationSteps = IndentationHelper.GetIndentationSteps(indentationOptions, attributeList);
var indentationTrivia = IndentationHelper.GenerateWhitespaceTrivia(indentationOptions, indentationSteps);
newRoot = newRoot.ReplaceNode(newRoot.GetCurrentNode(attributeList), GetNewAttributeList(attributeList, indentationTrivia));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Wouldn't it be more efficient to build a dictionary of replacements and then do a single ReplaceNodes call?

Copy link
Member Author

Choose a reason for hiding this comment

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

ReplaceNodes as far as I know does not support replacing one node with a set of new nodes. This is why I had to use ReplaceNode instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I forgot about that. I just had a look at the SyntaxRewriter as well and it also suffers from the same problem.
It could still be done using the parent node of the AttributeListSyntax, but that would be a large amount of work and I'm not sure if the performance gain is significant enough for that.

@vweijsters
Copy link
Contributor

👍 Looks good to me

@sharwell
Copy link
Member

sharwell commented Dec 4, 2015

👍

@sharwell sharwell added this to the 1.0.0 RC 2 milestone Dec 4, 2015
sharwell added a commit that referenced this pull request Dec 4, 2015
@sharwell
Merge pull request #1893 from pdelvo/fix-1879
b2c2f3b 
Implement a custom fix all provider for SA1133
@sharwell sharwell merged commit 1392c9e into DotNetAnalyzers:master Dec 4, 2015
@pdelvo pdelvo deleted the fix-1879 branch December 4, 2015 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@vweijsters vweijsters

Labels
None yet
Projects
None yet
Milestone
1.0.0 RC 2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pdelvo @codecov-io @vweijsters @sharwell