Skip to content

[New Analyzer/Fixer] Prefer static HashData method over ComputeHash #4797

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.

Sign up for GitHub

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

Jump to bottom
Merged
buyaa-n merged 46 commits into from
Oct 22, 2021
Merged

[New Analyzer/Fixer] Prefer static HashData method over ComputeHash #4797

buyaa-n merged 46 commits into from
Oct 22, 2021

Conversation

@wzchua
Copy link

@wzchua wzchua commented Feb 7, 2021 •
edited
Loading

@wzchua wzchua force-pushed the feature/prefer-static-hash-methods branch from b2cc096 to af5ce67 Compare February 7, 2021 02:38
@wzchua wzchua changed the base branch from release/6.0.1xx-preview1 to master February 7, 2021 02:41
@wzchua
Copy link
Author

wzchua commented Feb 7, 2021

I'm confused about the target branch. targeting master first for the CI

@wzchua wzchua force-pushed the feature/prefer-static-hash-methods branch 3 times, most recently from 8d2bce3 to b97cc5a Compare February 7, 2021 04:08
@codecov
Copy link

codecov bot commented Feb 7, 2021 •
edited
Loading

Codecov Report

Merging #4797 (05c9f32) into main (d581cc4) will increase coverage by 0.03%.
The diff coverage is 90.11%.

@@            Coverage Diff             @@
##             main    #4797      +/-   ##
==========================================
+ Coverage   95.52%   95.56%   +0.03%     
==========================================
  Files        1275     1280       +5     
  Lines      292855   296596    +3741     
  Branches    17691    18014     +323     
==========================================
+ Hits       279757   283432    +3675     
- Misses      10663    10716      +53     
- Partials     2435     2448      +13

@Youssef1313
Copy link
Member

Youssef1313 commented Feb 7, 2021

This should target a release/6.0.1xx branch. But not sure if it should target preview1 or preview2 branch. @mavasani to confirm the correct branch.

Youssef1313
Youssef1313 reviewed Feb 7, 2021
View reviewed changes
...nalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.Fixer.cs Outdated
Comment on lines 59 to 60
Copy link
Member

@Youssef1313 Youssef1313 Feb 7, 2021

Choose a reason for hiding this comment

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

@mavasani @Evangelink Should the title for the codefix start with "Use" instead (i.e. different from analyzer title)?

Youssef1313
Youssef1313 reviewed Feb 7, 2021
View reviewed changes
...nalyzers/Core/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHash.Fixer.cs Outdated
Copy link
Member

@Youssef1313 Youssef1313 Feb 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC, @sharwell said somewhere that equivalenceKey shouldn't be localized. So you should probably use nameof here. (I'm not sure though what problems can this cause)

Youssef1313
Youssef1313 reviewed Feb 7, 2021
View reviewed changes
...ualBasic/Microsoft.NetCore.Analyzers/Performance/BasicPreferHashDataOverComputeHash.Fixer.vb Outdated
Copy link
Member

@Youssef1313 Youssef1313 Feb 7, 2021

Choose a reason for hiding this comment

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

Not sure if the NotNullWhen attribute is necessary in VB?

Copy link
Author

@wzchua wzchua Feb 7, 2021

Choose a reason for hiding this comment

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

I'm not sure either. VS generated this

Youssef1313
Youssef1313 reviewed Feb 7, 2021
View reviewed changes
...zers/UnitTests/Microsoft.NetCore.Analyzers/Performance/PreferHashDataOverComputeHashTests.cs Outdated
Comment on lines 71 to 231
Copy link
Member

@Youssef1313 Youssef1313 Feb 7, 2021

Choose a reason for hiding this comment

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

I'm a little bit confused why there are 3 diagnostics here?

Copy link
Author

@wzchua wzchua Feb 7, 2021

Choose a reason for hiding this comment

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

I'm capturing AdditionalLocations for the fixer. Those show up in the diagnostics

@wzchua wzchua changed the base branch from master to merges/master-to-release/6.0.1xx-preview2 February 12, 2021 14:13
@wzchua wzchua marked this pull request as ready for review February 12, 2021 14:14
@wzchua wzchua requested a review from a team as a code owner February 12, 2021 14:14
@wzchua wzchua changed the base branch from merges/master-to-release/6.0.1xx-preview2 to release/6.0.1xx-preview2 February 12, 2021 14:15
carlossanlop
carlossanlop reviewed Mar 9, 2021
View reviewed changes
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated
Copy link

@carlossanlop carlossanlop Mar 9, 2021

Choose a reason for hiding this comment

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

I think this description should also give a short explanation of why it is simpler.

carlossanlop
carlossanlop reviewed Mar 9, 2021
View reviewed changes
src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md Outdated
Copy link

@carlossanlop carlossanlop Mar 9, 2021

Choose a reason for hiding this comment

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

Same comment about expanding the description to explain why.

@wzchua wzchua changed the base branch from release/6.0.1xx to release/7.0.1xx September 12, 2021 13:04
@wzchua
Copy link
Author

wzchua commented Sep 12, 2021

Is it alright if I just squashed the commits into 1? It's getting messy imo

@wzchua wzchua requested a review from buyaa-n September 13, 2021 13:11
buyaa-n
buyaa-n approved these changes Sep 16, 2021
View reviewed changes
Copy link

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Thank you @wzchua for your contribution, and applying the comments promptly, left few NITs, overall looks great to me 🎉

CC @mavasani @jmarolf

@jmarolf jmarolf changed the base branch from release/7.0.1xx to main September 28, 2021 22:03
@buyaa-n buyaa-n enabled auto-merge (squash) October 21, 2021 22:03
@buyaa-n
Copy link

buyaa-n commented Oct 21, 2021

Enabled auto-merge, if @mavasani @jmarolf have a comment we can address them with different PR

@buyaa-n buyaa-n merged commit 9cd12bf into dotnet:main Oct 22, 2021
@github-actions github-actions bot added this to the vNext milestone Oct 22, 2021
@jeffhandley
Copy link
Member

jeffhandley commented Oct 22, 2021

Hooray! Thanks for the contribution @wzchua. 👍

@wzchua wzchua mentioned this pull request Oct 23, 2021
Merged
@wzchua wzchua deleted the feature/prefer-static-hash-methods branch October 23, 2021 09:44
@jmarolf jmarolf modified the milestones: vNext, .NET 7.x Preview 1 Nov 3, 2021
@jmarolf jmarolf mentioned this pull request Jan 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bartonjs bartonjs bartonjs left review comments

+5 more reviewers

@carlossanlop carlossanlop carlossanlop left review comments

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks left review comments

@mavasani mavasani mavasani left review comments

@Youssef1313 Youssef1313 Youssef1313 left review comments

@buyaa-n buyaa-n buyaa-n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlop carlossanlop

@mavasani mavasani

@wzchua wzchua

Labels

None yet

Projects

None yet

Milestone

.NET 7.x Preview 1

Development

Successfully merging this pull request may close these issues.

Analyzer / fixer proposal: Prefer static HashData methods over ComputeHash

9 participants

@wzchua @Youssef1313 @carlossanlop @mavasani @jeffhandley @buyaa-n @GrabYourPitchforks @bartonjs @jmarolf