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

Separate out CA1307 into two rules #4043

Merged
merged 1 commit into from
Aug 20, 2020
Merged

Conversation

mavasani
Copy link
Contributor

Fixes #2581, again!

  1. CA1310: Correctness rule that only flags string compare methods that are known to use culture specific string comparison by default
  2. CA1307: Maintainability/Readability rule that flags the remainder methods that have an overload with an additional StringComparison parameter at the end. This rule does not care about the default string comparison used by the API.

Fixes dotnet#2581, again!

1. CA1310: Correctness rule that only flags string compare methods that are known to use culture specific string comparison by default
2. CA1307: Maintainability/Readability rule that flags the remainder methods that have an overload with an additional StringComparison parameter at the end. This rule does not care about the defaault string comparison used by the API.
@mavasani mavasani requested a review from dotpaul August 19, 2020 23:30
@mavasani mavasani requested a review from a team as a code owner August 19, 2020 23:30
@mavasani
Copy link
Contributor Author

Copy link
Contributor

@dotpaul dotpaul left a comment

Choose a reason for hiding this comment

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

🚢

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #4043 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4043      +/-   ##
==========================================
- Coverage   95.63%   95.63%   -0.01%     
==========================================
  Files        1156     1156              
  Lines      256670   256749      +79     
  Branches    15367    15370       +3     
==========================================
+ Hits       245474   245549      +75     
- Misses       9242     9243       +1     
- Partials     1954     1957       +3     

@mavasani
Copy link
Contributor Author

Thanks @dotpaul

@mavasani mavasani merged commit cd93448 into dotnet:master Aug 20, 2020
@mavasani mavasani deleted the FixCA1307 branch August 20, 2020 00:16
@mavasani
Copy link
Contributor Author

@GrabYourPitchforks
Copy link
Member

Doesn't this mean that CA1307 will now always fire on string.GetHashCode(), string.IndexOf(char), and similar? I fear this will end up introducing significant noise into the system.

@mavasani
Copy link
Contributor Author

Yes, if you only care about correctness, you should disable CA1307 and just enable CA1310. CA1307 would most likely be used like a refactoring/hidden/suggestion available in IDE for users where they want to opportunistically keep moving to API overloads with StringComparison parameter.

@KrisVandermotten
Copy link

Looks good, thanks!

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.

False positive in CA1307 for string.IndexOf(char)
4 participants