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

Add MakeTypesInternalAnalyzer #6820

Merged
merged 10 commits into from
Sep 25, 2023
Merged

Add MakeTypesInternalAnalyzer #6820

merged 10 commits into from
Sep 25, 2023

Conversation

CollinAlpert
Copy link
Contributor

@CollinAlpert CollinAlpert requested a review from a team as a code owner July 31, 2023 10:52
@codecov
Copy link

codecov bot commented Jul 31, 2023 •
edited
Loading

Codecov Report

Merging #6820 (bda3e0b) into main (39ccb5b) will increase coverage by 0.00%.
The diff coverage is 99.30%.

@@           Coverage Diff            @@
##             main    #6820    +/-   ##
========================================
  Coverage   96.42%   96.43%            
========================================
  Files        1401     1408     +7     
  Lines      334973   335687   +714     
  Branches    11072    11081     +9     
========================================
+ Hits       322997   323704   +707     
- Misses       9193     9198     +5     
- Partials     2783     2785     +2

Youssef1313
Youssef1313 reviewed Aug 3, 2023
View reviewed changes
src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/MakeTypesInternal.cs Outdated
Comment on lines 40 to 41
compilationStartContext.RegisterSyntaxNodeAction(AnalyzeTypeDeclaration, TypeKinds);
compilationStartContext.RegisterSyntaxNodeAction(AnalyzeEnumDeclaration, EnumKind);
Copy link
Member

Choose a reason for hiding this comment

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

delegate declaration as well?

@CollinAlpert
Copy link
Contributor Author

@Youssef1313 I have addressed your feedback.

@mavasani
Copy link
Contributor

LGTM from a quick glance.

One suggestion: Analyzers that operate/bail out based on project output kind should probably be configurable to allow end users to override the applicable output kinds, we added such an option for CA2007: https://github.com/dotnet/roslyn-analyzers/blob/main/docs/Analyzer%20Configuration.md#analyzed-output-kinds

buyaa-n
buyaa-n reviewed Sep 23, 2023
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Thank you @CollinAlpert, left a few comments, overall looks good.

...zers/CSharp/Microsoft.CodeQuality.Analyzers/Maintainability/CSharpMakeTypesInternal.Fixer.cs Outdated Show resolved Hide resolved
...zers/CSharp/Microsoft.CodeQuality.Analyzers/Maintainability/CSharpMakeTypesInternal.Fixer.cs Outdated Show resolved Hide resolved
...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx Outdated Show resolved Hide resolved
...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx Outdated Show resolved Hide resolved
...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx Outdated Show resolved Hide resolved
...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx Outdated Show resolved Hide resolved
...nalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/MakeTypesInternalTests.cs Show resolved Hide resolved
@CollinAlpert
Merge branch 'master' into issue_78388
bc210a0 
# Conflicts:
#	src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
#	src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md
#	src/NetAnalyzers/RulesMissingDocumentation.md
@CollinAlpert
Applied code review suggestions.
075c130
@CollinAlpert
Copy link
Contributor Author

@mavasani @buyaa-n Thank you for your feedback, I have addressed it.

buyaa-n
buyaa-n reviewed Sep 25, 2023
View reviewed changes
src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/MakeTypesInternal.cs Outdated
SyntaxTree? firstSyntaxTree;
if (compilation.Options.OutputKind is not (OutputKind.ConsoleApplication or OutputKind.WindowsApplication or OutputKind.WindowsRuntimeApplication)
|| (firstSyntaxTree = context.Compilation.SyntaxTrees.FirstOrDefault()) is null
|| !context.Options.GetOutputKindsOption(Rule, firstSyntaxTree, compilation).Contains(compilation.Options.OutputKind))
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making OutputKind configurable, though looks with the current logic only OutputKind.ConsoleApplication, OutputKind.WindowsApplication or OutputKind.WindowsRuntimeApplication are configurable, there is no way to enable the rule for OutputKind.DynamicallyLinkedLibrary. I think if it is giving an option to configure, all should be configurable, what you think @CollinAlpert @stephentoub?

Also please add the rule into Configurable Rules list in https://github.com/dotnet/roslyn-analyzers/blob/main/docs/Analyzer%20Configuration.md#analyzed-output-kinds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it is better to make all OutputKinds configurable.

gewarren
gewarren reviewed Sep 25, 2023
View reviewed changes
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

I left a couple suggestions for the resource file.

...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx Outdated Show resolved Hide resolved
...etAnalyzers/Core/Microsoft.CodeQuality.Analyzers/MicrosoftCodeQualityAnalyzersResources.resx Outdated Show resolved Hide resolved
@buyaa-n
Copy link
Contributor

buyaa-n commented Sep 25, 2023

I left a couple suggestions for the resource file.

Great, thank you @gewarren!

buyaa-n
buyaa-n approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you @CollinAlpert!

@buyaa-n buyaa-n enabled auto-merge (squash) September 25, 2023 20:45
@buyaa-n buyaa-n merged commit b39866e into dotnet:main Sep 25, 2023
11 checks passed
@CollinAlpert CollinAlpert deleted the issue_78388 branch September 26, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@gewarren gewarren gewarren left review comments

@buyaa-n buyaa-n buyaa-n approved these changes

Assignees

@buyaa-n buyaa-n

Labels
Community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Analyzer] Make types declared in an executable internal
5 participants
@CollinAlpert @mavasani @buyaa-n @gewarren @Youssef1313