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

Breaking API change to support base type comparison. #45

Merged
merged 2 commits into from
Jan 8, 2023

Conversation

diegofrata
Copy link
Owner

Equals made protected and IEquatable made explicit for classes. This should sort out the last issue with comparing derived types cast as base-type.

Solves #43

@diegofrata
Copy link
Owner Author

diegofrata commented Jan 3, 2023

@christianrondeau are you happy with this fix? I have added a new test case specific to the stack overflow issue you had:

https://github.com/diegofrata/Generator.Equals/blob/575c48698102113f12334170b38ae7ec777dbbdd/Generator.Equals.Tests/Classes/OverridingEquals.Sample.cs

Everything looks good to me. If it looks good to you, then I will cut 3.0.0 so you can carry on using it.

@christianrondeau
Copy link

christianrondeau commented Jan 6, 2023

I'm looking at the branch, is that the eventual 3.0? And FYI I got this when referencing directly from source, it's annoying when you don't have VS installed :D

image

You could check for an environment variable, or a precompiler directive other than DEBUG.

So, I had this that you may find interesting:

new protected member declared in sealed type

If you add a test case with a sealed class using Equatable, it'll say that you may as well mark the method as private.

Which also makes me realize you should add <generated /> at the beginning of your generated files so the IDE knows it's a generated file.

@diegofrata
Copy link
Owner Author

@christianrondeau I have fixed the private/protected modifier and added extra tests. On the <generated/> comment I could not find any guidelines or docs for use -- I am not entirely sure this does anything that's not already accomplished by the GeneratedCodeAttribute.

If you have any pointers to it, let me know.

@diegofrata
Copy link
Owner Author

These changes are available on 3.0.0-beta001 if you want to give that a go!

@christianrondeau
Copy link

You may be right, anyway I also got it wrong, it's actually <auto-generated />
https://learn.microsoft.com/en-us/dotnet/csharp/roslyn-sdk/source-generators-overview does it but doesn't explain why... I guess it's just to make it clear. But I guess it doesn't really matters.

I'll pull and try it when I have some time :)

@diegofrata diegofrata changed the base branch from main to develop January 8, 2023 00:57
@diegofrata diegofrata merged commit 95b4e5a into develop Jan 8, 2023
@diegofrata diegofrata deleted the deep-equality branch January 8, 2023 00:57
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

2 participants