-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Enable .NET analyzers and resolve warnings #2757
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
Enable .NET analyzers and resolve warnings #2757
Conversation
|
@martincostello is this one good to go? |
|
If people are happy with the direction, yes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables .NET code analyzers in Release configuration and addresses analyzer warnings to improve code quality. The changes focus on optimizing logging performance, fixing incorrect exception usage, and applying best practices.
Key Changes:
- Added
AnalysisMode>Minimum</AnalysisMode>to enable .NET analyzers in Release builds for cart and accounting services - Converted string interpolation to structured logging with proper placeholders and added
IsEnabledchecks to avoid unnecessary work - Changed
ArgumentNullExceptiontoInvalidOperationExceptionfor environment variable validation (correct exception type for missing configuration)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/cart/Directory.Build.props | Enables minimum analysis mode for Release configuration |
| src/accounting/Directory.Build.props | Enables minimum analysis mode for Release configuration |
| src/cart/src/cartstore/ValkeyCartStore.cs | Converts logging to structured format with IsEnabled guards for performance optimization |
| src/accounting/Consumer.cs | Fixes exception type, converts to structured logging, and makes BuildConsumer method static |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I'm okay with it! Would you be able to take a look at Copilot review and check if it makes sense? |
Done.
I imagine someone's enabled the "ask for Copilot reviews" ruleset. |
|
Well that last quick edit proved-out the CI failure 😅 |
Enable .NET analyzers and address recommendations to avoid redundant work, formatted strings, incorrect use of `ArgumentNullException` and avoiding an instance method that does not access state.
Update two placeholders for clarity and consistency.
c46cc8f to
916dc05
Compare
Fix two additional CA1873 warnings.
* Enable .NET analyzers Enable .NET analyzers and address recommendations to avoid redundant work, formatted strings, incorrect use of `ArgumentNullException` and avoiding an instance method that does not access state. * Update placeholders Update two placeholders for clarity and consistency. * Fix CA1873 warnings Fix two additional CA1873 warnings.
Changes
Enable .NET analyzers and address recommendations to:
ArgumentNullException;Running
dotnet build --configuration Releasewith these changes has no compiler errors.Relates to #2413 (comment).
/cc @martinjt
Merge Requirements
For new features contributions, please make sure you have completed the following
essential items:
CHANGELOG.mdupdated to document new feature additionsMaintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.