Add sync namespace analyzer #49990
Merged
Add sync namespace analyzer #49990
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoeRobich
reviewed
Dec 17, 2020
src/Analyzers/CSharp/Analyzers/NamespaceFileSync/NamespaceFileSyncDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
mavasani
reviewed
Dec 17, 2020
mavasani
reviewed
Dec 17, 2020
vritant24
reviewed
Dec 17, 2020
src/Analyzers/CSharp/Analyzers/NamespaceFileSync/NamespaceFileSyncDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
vritant24
reviewed
Dec 17, 2020
src/Analyzers/CSharp/Analyzers/NamespaceFileSync/NamespaceFileSyncDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/NamespaceFileSync/NamespaceFileSyncDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
davidwengier
approved these changes
Dec 18, 2020
src/Analyzers/CSharp/Analyzers/NamespaceFileSync/NamespaceFileSyncDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/NamespaceFileSync/NamespaceFileSyncDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/NamespaceFileSync/NamespaceFileSyncDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/PathMetadataUtilities.cs
Outdated
Show resolved
Hide resolved
genlu
reviewed
Dec 18, 2020
src/Analyzers/CSharp/Analyzers/NamespaceFileSync/NamespaceFileSyncDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
genlu
reviewed
Dec 18, 2020
| // TODO : can this cause a loop? This condition was used by the service before, | ||
| // but now could possibly add an Error severity as an analyzer | ||
| var isCurrentNamespaceInvalid = namespaceDeclaration.Name | ||
| .GetDiagnostics().Any(d => d.Severity == DiagnosticSeverity.Error); |
There was a problem hiding this comment.
Maybe check for DefaultSeverity + id?
There was a problem hiding this comment.
this feels like an odd check. i'm not sure what it's really trying to ask
There was a problem hiding this comment.
I'm not 100% sure this is needed. Even if the namespace is invalid, is it practical to suggest the correct name?
There was a problem hiding this comment.
Ping on this. Intent was to check that the namespace was valid before suggesting to fix. Thoughts on what the right behavior should be?
Co-authored-by: Manish Vasani <[email protected]> Co-authored-by: David Wengier <[email protected]>
...ortable/CodeRefactorings/SyncNamespace/AbstractSyncNamespaceCodeRefactoringProvider.State.cs
Show resolved
Hide resolved
CyrusNajmabadi
requested changes
Feb 1, 2021
| where TNamespaceSyntax : SyntaxNode | ||
| { | ||
| private static readonly LocalizableResourceString s_localizableTitle = new( | ||
| nameof(AnalyzersResources.Namespace_does_not_match_folder_structure), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)); |
| nameof(AnalyzersResources.Namespace_does_not_match_folder_structure), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)); | ||
|
|
||
| private static readonly LocalizableResourceString s_localizableInsideMessage = new( | ||
| nameof(AnalyzersResources.Namespace_0_does_not_match_folder_structure_expected_1), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)); |
There was a problem hiding this comment.
nit: consider inlining these two statics. up to you, but it's waht we do in lots of other places.
|
|
||
| private static readonly SymbolDisplayFormat s_namespaceDisplayFormat = SymbolDisplayFormat | ||
| .FullyQualifiedFormat | ||
| .WithGlobalNamespaceStyle(SymbolDisplayGlobalNamespaceStyle.Omitted); |
There was a problem hiding this comment.
nit: place statics above constructor.
| return; | ||
| } | ||
|
|
||
| if (context.Node is TNamespaceSyntax namespaceDecl) |
There was a problem hiding this comment.
my preference would be a hard-cast here as this shouldn't be possible to fail.
| /// on a sequential level instead of batch fixing and merging the changes. This prevents | ||
| /// collissions that the batch fixer won't handle correctly but is slower. | ||
| /// </summary> | ||
| internal abstract partial class AbstractChangeNamespaceToMatchFolderCodeFixProvider |
There was a problem hiding this comment.
this name is so much better. t hanks.
| public MyCodeAction(string title, Func<CancellationToken, Task<Solution>> createChangedSolution) | ||
| : base(title, createChangedSolution) | ||
| { | ||
|
|
CyrusNajmabadi
approved these changes
Feb 6, 2021
ghost
merged commit 
ghost
modified the milestones:
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sync namespace analyzer will warn and provide code fixes for when namespaces do not match the folder structure. It should follow the same logic as https://docs.microsoft.com/en-us/visualstudio/ide/reference/sync-namespace-and-folder-name?view=vs-2019 but will allow users to configure warning severity and have CI integration.