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

Make CA1827 fire for 'is' patterns. #7213

Merged
merged 5 commits into from
Mar 1, 2024
Merged

Conversation

CollinAlpert
Copy link
Contributor

Fixes #7207

@CollinAlpert CollinAlpert requested a review from a team as a code owner February 28, 2024 22:36
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

Attention: Patch coverage is 98.48485% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 96.46%. Comparing base (54ba3c9) to head (3100994).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7213    +/-   ##
========================================
  Coverage   96.46%   96.46%            
========================================
  Files        1427     1427            
  Lines      341675   341928   +253     
  Branches    11259    11268     +9     
========================================
+ Hits       329581   329846   +265     
+ Misses       9245     9232    -13     
- Partials     2849     2850     +1     

@@ -314,6 +321,48 @@ private static bool AnalyzeParentInvocationOperation(IInvocationOperation parent
return true;
}

private static bool AnalyzeIsPatternOperation(IIsPatternOperation isPatternOperation, out bool shouldNegateIsEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment about the return values of this method.


if (isPatternOperation.Pattern is INegatedPatternOperation negatedPattern)
{
if (negatedPattern.Pattern is IRelationalPatternOperation { OperatorKind: BinaryOperatorKind.GreaterThan, Value: ILiteralOperation { ConstantValue: { HasValue: true, Value: 0 } } }
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest adding comments for each of these cases where you have the code corresponding to the condition for easy understanding, probably you can just copy over the corresponding test case code that you have added for each of the conditional branches in this method.

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

The change looks good. Kindly add some comments and we can get this merged. Thanks!

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Thanks!

@mavasani mavasani enabled auto-merge March 1, 2024 10:16
@mavasani mavasani merged commit 59b2d9d into dotnet:main Mar 1, 2024
11 checks passed
@CollinAlpert CollinAlpert deleted the issue-7207 branch March 1, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CA1827 "Do not use Count() when Any() can be used" in patterns
2 participants