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

Add PreferTypedStringBuilderAppendOverloads analyzer + fixer #3443

Merged
merged 1 commit into from
Mar 30, 2020

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub requested a review from a team as a code owner March 27, 2020 21:52
@codecov
Copy link

codecov bot commented Mar 27, 2020

Codecov Report

Merging #3443 into master will increase coverage by 0.00%.
The diff coverage is 98.40%.

@@           Coverage Diff            @@
##           master    #3443    +/-   ##
========================================
  Coverage   95.27%   95.28%            
========================================
  Files        1006     1009     +3     
  Lines      230946   231260   +314     
  Branches    14921    14939    +18     
========================================
+ Hits       220041   220346   +305     
- Misses       9259     9263     +4     
- Partials     1646     1651     +5     

@mavasani
Copy link
Contributor

FYI: You will need to fix a new RS diagnostic once #3444 goes in - should be easy to fix with an IDE code fix.

@stephentoub
Copy link
Member Author

FYI: You will need to fix a new RS diagnostic once #3444 goes in - should be easy to fix with an IDE code fix.

Done.

@stephentoub stephentoub merged commit d02078f into dotnet:master Mar 30, 2020
@stephentoub stephentoub deleted the stringbuilderoverloads branch March 30, 2020 21:22
@carlossanlop
Copy link
Member

carlossanlop commented Mar 30, 2020

I have a question about this PR, @stephentoub:

The original issue was categorized by Immo as "Performance", but this PR adds the analyzer inside the Runtime folder instead of the Performance folder. Why is that?

  • Microsoft.CodeAnalysis.NetAnalyzers project
    • Microsoft.NetCore.Analyzers folder
      • Runtime folder <-
      • Performance folder

I have this issue assigned to me to create an analyzer, and was also categorized as "Performance", so I'd like to know how to decide in which folder should I create my analyzer.

[InlineData("object", false)]
[InlineData("char[]", false)]
[InlineData("DateTime", false)]
[InlineData("DayOfWeek", false)]
Copy link
Member

Choose a reason for hiding this comment

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

Is this to cover the generalized enum scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup.

@stephentoub
Copy link
Member Author

The original issue was categorized by Immo as "Performance", but this PR adds the analyzer inside the Runtime folder instead of the Performance folder. Why is that?

The categorization that's important is this one:
https://github.com/dotnet/roslyn-analyzers/pull/3443/files#diff-d6a8b8e17ba2b3ecec87bd042b598259R22
The folder isn't publicly visible for folks consuming the analyzer. I put it in the runtime folder because the general organization of this project is generally around types rather than purpose. But we can always move things between folders as needed. @mavasani, any further advice?

@mavasani
Copy link
Contributor

Agree with @stephentoub - the important bit is the category set in the descriptor. Ideally, the folder structure will match the rule category, but it has been out of sync for a while. We have wanted to correct it, but never got to prioritize that work. We can even consider flattening the files into a single folder, i.e. put all analyzers under the folder https://github.com/dotnet/roslyn-analyzers/tree/master/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers and remove all sub-folders.

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.

Prefer typed overload on StringBuilder.Append()
5 participants