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

Analyzer for SameAs when used on Value Types. #278

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

manfred-brands
Copy link
Member

@manfred-brands manfred-brands commented Aug 8, 2020

CodeFix to replace SameAs with IsEqualTo
Fixes #276

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

Good work @manfred-brands. I've only found a handful of nitpick and nothing major.

documentation/NUnit2040.md Outdated Show resolved Hide resolved
documentation/NUnit2040.md Outdated Show resolved Hide resolved
@manfred-brands
Copy link
Member Author

Thanks @mikkelbu for the review. Review changes applied and squashed. Ready for merge.

@jnm2
Copy link
Contributor

jnm2 commented Aug 8, 2020

What do you think about handling Assert.AreSame? Should the only diagnostic there be recommending that you switch to using the constraint model, and only then you see the problem that you're using value types?

@manfred-brands
Copy link
Member Author

@jnm2 I did think of that and assumed people would convert first. But what if they disable the Classic To Constraint rules then they would get no warning.
The same holds for SameAsIncompatibleTypesAnalyzerTests which also only works on the Constraint Model.
But ConstActualValueUsageAnalyzer works on both.

@manfred-brands
Copy link
Member Author

@mikkelbu I changed the Identifier from NUnit2040 into NUnit1029. The 2k range is used for ClassicAssert conversions.

@manfred-brands
Copy link
Member Author

Updated the branch to support the Classic Assert.AreSame and Assert.AreNotSame methods including matching code fix to convert to AreEqual and AreNotEqual respectively.

@Dreamescaper
Copy link
Member

@mikkelbu I changed the Identifier from NUnit2040 into NUnit1029. The 2k range is used for ClassicAssert conversions.

As far as I remember 1k range is used for Structure analyzers, and 2k - for Assertion analyzers.
I think that 2k is more applicable here.

@manfred-brands
Copy link
Member Author

I see now that the SameAsIncompatibleTypesAnalyzer uses 2020. Reverted the commit.

@mikkelbu
Copy link
Member

@Dreamescaper Is right (I plan to make this more clear in the TOC by having two separate tables.

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

@manfred-brands It looks good to me (and good that you also solved the classic case), so I'm happy to merge it as it is, but if you want to merge commits or similar then please tell me.

@manfred-brands
Copy link
Member Author

@mikkelbu Branch updates. Ready for merge again.

@mikkelbu
Copy link
Member

Thanks for the work @manfred-brands, I'll merge it now.

@mikkelbu mikkelbu merged commit f806b8c into nunit:master Aug 11, 2020
@mikkelbu mikkelbu added this to the Release 0.5 milestone Aug 11, 2020
@manfred-brands manfred-brands deleted the 276_SameAsOnValueTypes branch August 12, 2020 00:05
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.

SameAs should warn if used with value types
4 participants