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

feat(analyzer): add rules regarding model name suffixes #6916

Merged
merged 21 commits into from
Oct 26, 2023

Conversation

archerzz
Copy link
Member

@archerzz archerzz commented Sep 7, 2023

  • add four rules checking the suffixes of model names
  • upgrade Roslyn to latest version since new rules require some API
  • minor refactor existing codes
  • add test cases for the new rules

resolve #6905

- add four rules checking the suffixes of model names
- upgrade Roslyn to latest version since new rules require some API
- minor refactor existing codes
- add test cases for the new rules

resolve Azure#6905
@archerzz archerzz force-pushed the net-analyzer/new-rules-from-mgmt branch from 94bfbb0 to 2e313f4 Compare September 7, 2023 08:35
@archerzz
Copy link
Member Author

archerzz commented Sep 7, 2023

preview pr on azure-sdk-for-net: Azure/azure-sdk-for-net#38560

@m-nash ^^

{
protected static readonly string Title = "Improper model name suffix";
protected static readonly string Description = "Suffix is not recommended. Consider to remove or modify it.";
protected static readonly string GeneralRenamingMessageFormat = "Model name '{0}' ends with '{1}'. Suggest to rename it to an appropriate name.";
Copy link
Member

Choose a reason for hiding this comment

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

Should this link to our guidelines that specify the naming rules we are enforcing here?

Copy link
Member Author

@archerzz archerzz Sep 14, 2023

Choose a reason for hiding this comment

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

I could not find the official guidelines. We do have a OneNote page on that. @m-nash Shall we update the .NET Azure SDK Design Guidelines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or we can do something like StyleCop.

@archerzz
Copy link
Member Author

archerzz commented Sep 14, 2023

@christothes @m-nash Changes after last review:

  • replace virtual unimplemented methods with abstract methods
  • performance improvement:
    • replace regular expression with string for suffix search
    • use Span to improve string manipulation performance
    • avoid extra string allocation
    • improve algorithm in several places
  • relax conditions of searching model classes, now if a class contains either serialization method or a deserialization method, it's a model class. That resolves cases that model classes are not put under Models namespace.
  • exclude non-public types from analysis
  • fix an index out of range exception
  • fix comments
  • downgrade to Roslyn 4.4.0.0 which aligns with the version used in azure-sdk-for-net and fixes CS9057
  • suppress AD0001, after some google search, it looks like an issue in Roslyn analyzers in version 4.4.0.0. If we chose 4.7.0.0 that problem would disappear.
CSC : warning AD0001: Analyzer 'Microsoft.CodeAnalysis.Analyzers.MetaAnalyzers.DiagnosticDescriptorCreationAnalyzer' threw an exception of type 'System.ArgumentException' with message 'Syntax node is not withi
n syntax tree'. [C:\Users\mingzhehuang\workspaces\archerzz\azure-sdk-tools\src\dotnet\Azure.ClientSdk.Analyzers\Azure.ClientSdk.Analyzers\Azure.ClientSdk.Analyzers.csproj]

Updated preview PR on azure-sdk-for-net: Azure/azure-sdk-for-net#38705

Mingzhe Huang (from Dev Box) added 4 commits October 25, 2023 12:01
@archerzz
Copy link
Member Author

archerzz commented Oct 26, 2023

Changes since last review:

  • replace List and Join with StringBuilder
  • adopt ArrayPool to collect namespaces
  • merge latest main branch
  • extract analyzer descriptors to align with other analyzers

Preview PR: Azure/azure-sdk-for-net#39518 It's the same as the previous one, except that analyzer package is the latest.

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.

add new .Net SDK analyzer rules based on mgmt SDK review experience
3 participants