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

CA2246 Warning for unobvious assignment #2717

Merged
merged 19 commits into from Sep 12, 2019
Merged

CA2246 Warning for unobvious assignment #2717

merged 19 commits into from Sep 12, 2019

Conversation

maxkoshevoi
Copy link
Contributor

@maxkoshevoi maxkoshevoi commented Jul 29, 2019

This is related to issue #2527

I've done what I can here. This is my first time contributing to large repository, so I would love to hear any feedback.

I think name of the analyzer could be shorter and more understandable.

Here's how warning currently looks (C is class and S is struct):
image

Thanks!

@dnfclas
Copy link

dnfclas commented Jul 29, 2019

CLA assistant check
All CLA requirements met.

@mavasani mavasani self-assigned this Aug 5, 2019
Copy link
Member

@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.

Overall, looks good. @maxkoshevoi would you be able to address the review comments?

Also tagging @dotnet/roslyn-analysis for an additional review of the PR and proposed rule itself.

@maxkoshevoi
Copy link
Contributor Author

maxkoshevoi commented Aug 19, 2019

Overall, looks good. @maxkoshevoi would you be able to address the review comments?

Yes, I'll do it in the next couple of days

Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

I suggested a new description - let me know if I got the logic correct.

Co-Authored-By: Genevieve Warren <gewarren@microsoft.com>
@mavasani
Copy link
Member

mavasani commented Sep 6, 2019

@maxkoshevoi I can merge the PR once you fix up the resource strings and xlf files.

@maxkoshevoi
Copy link
Contributor Author

All done. Thanks a lot for your help!

@mavasani
Copy link
Member

mavasani commented Sep 6, 2019

@maxkoshevoi There seem to be merge conflicts in src/Microsoft.CodeQuality.Analyzers/Core/QualityGuidelines/MicrosoftQualityGuidelinesAnalyzersResources.resx. Please fix them and rebuild to regenerate xlf files.

@mavasani mavasani closed this Sep 7, 2019
@mavasani mavasani reopened this Sep 7, 2019
@maxkoshevoi
Copy link
Contributor Author

It's strange. I don't see and conflicts from my end:

image

PS: I've rebuild solution after merge and still there were no changes to commit

@mavasani
Copy link
Member

mavasani commented Sep 7, 2019

@maxkoshevoi Did you sync the latest sources from master?

@maxkoshevoi
Copy link
Contributor Author

maxkoshevoi commented Sep 7, 2019 via email

@mavasani
Copy link
Member

@maxkoshevoi I figured out the reason for your merge conflicts. Your PR is updating src/Microsoft.CodeQuality.Analyzers/Core/QualityGuidelines/MicrosoftQualityGuidelinesAnalyzersResources.resx , but that resource file no longer exists in master. The resource strings from that resx file have been moved to https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeQuality.Analyzers/Core/MicrosoftCodeQualityAnalyzersResources.resx

Can you merge latest master sources and move your added resource strings to the new resx file? Thanks!

@maxkoshevoi
Copy link
Contributor Author

Oh, now I see it. For some reason forked repository didn't see master from this repository (or something like that, I don't really know). I'm able to perform merge by switching to this request's brunch:
image

@maxkoshevoi
Copy link
Contributor Author

maxkoshevoi commented Sep 12, 2019 via email

@mavasani
Copy link
Member

@maxkoshevoi Can you make the analyzer itself be C# only then?

Copy link
Member

@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 for the contribution!

@mavasani
Copy link
Member

Filed MicrosoftDocs/visualstudio-docs#3916 to track adding documentation for the rule and #2847 to track updating analyzer's helpLinkUri once the documentation has been added.

@maxkoshevoi maxkoshevoi changed the title 2527 Warning for unobvious assignment CA2246 Warning for unobvious assignment Feb 19, 2021
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.

None yet

4 participants