Don't trigger naming style violation for prefixed numbers.#48306
Conversation
| name = StripCommonPrefixes(name.Substring(Prefix.Length), out var prefix); | ||
|
|
||
| if (prefix != string.Empty) | ||
| if (prefix != string.Empty && !char.IsDigit(name[0])) |
There was a problem hiding this comment.
not sure if this is ok. i.e. if would thinkg _0foo was ok. i think it should check all the remainder of hte string and only ignore if it's all a number.
There was a problem hiding this comment.
would also need a comment explaining this.
There was a problem hiding this comment.
Perhaps this helper method from Remove Unused Parameter could be moved so it can be re-used?
https://github.com/dotnet/roslyn/blob/master/src/Analyzers/Core/Analyzers/RemoveUnusedParametersAndValues/AbstractRemoveUnusedParametersAndValuesDiagnosticAnalyzer.cs#L327-L334
This allows for only one digit after an underscore, but at least we'd have consistency.
There was a problem hiding this comment.
Thanks for locating the code @davidwengier . Resolving the conflicting nature of the two rules was my goal with #47569
|
We'll take this one too design review tomorrow and make sure it's the direction we want. |
| /// are optionally followed by an integer, such as '_', '_1', '_2', etc. | ||
| /// These are treated as special discard symbol names. | ||
| /// </summary> | ||
| internal static bool IsSymbolWithSpecialDiscardName(ISymbol symbol) |
There was a problem hiding this comment.
Can we instead make this as extension method on ISymbol and move it to https://github.com/dotnet/roslyn/blob/master/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs?
There was a problem hiding this comment.
After I moved, IntelliSense says that the extension is available for Features, but not CodeStyle project.
There was a problem hiding this comment.
This extension belongs to Workspace, thus unavailable from CodeStyle. Since CodeStyle is being refactored out, I think there should be another suitable shared position.
There was a problem hiding this comment.
Which project/file did you move the extension to? Please ensure the file is moved to the shared project at the location mentioned above, which is imported into both CodeStyle and Workspaces layer.
There was a problem hiding this comment.
I moved to wrong file, with the same namespace Microsoft.CodeAnalysis.Shared.Extensions. It really confuses me that there are so many files named ISymbolExtension.
There was a problem hiding this comment.
Yeah, but Roslyn has multiple layers, each requiring its own set of extensions, so it is unavoidable.
mavasani
left a comment
There was a problem hiding this comment.
LGTM. Once the helper method is made an extension method, I can merge the PR.
|
Hello @mavasani! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Fixes #47569
The rule is quite naive and I'm not sure if it's appropriate.