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

Possibly unintended breaking change between 2.0.0 and 4.2.1 #54

Closed
asgerhallas opened this issue Nov 30, 2022 · 4 comments
Closed

Possibly unintended breaking change between 2.0.0 and 4.2.1 #54

asgerhallas opened this issue Nov 30, 2022 · 4 comments

Comments

@asgerhallas
Copy link

The following test passed in 2.0.0 but fails in 4.2.1 as it returns a Pass now. Is this intended?

    [Fact]
    public void ComparingObjectsWithNoProperties_DifferentType()
    {
        var comparer = (IComparison)new ComparisonBuilder().Create();
        var context = new ComparisonContext();

        Assert.Equal(
            ComparisonResult.Inconclusive, 
            comparer.Compare(context, new object(), new Blah()).result);
    }
    
    public class Blah { }
@asgerhallas
Copy link
Author

I suspect it is this addition that makes the difference:

if (propertyMap.Count == 0) return (ComparisonResult.Pass, context);

@asgerhallas
Copy link
Author

Allow me to bump this question :)

Is it intentional that two classes of different types, but both with no properties, should return a Pass?

I'd say it would be more natural with Inconclusive, to be able to fall back to the DefaultComparer.

@jamesfoster
Copy link
Owner

Hi, apologies for abandoning issues on this project for so long.

I believe this was an intended breaking change. If 2 types have no properties to compare (because they've all been ignored or are simply empty objects) then they should be treated as equal.

I do try to use semantic versioning, so if the major version number changes - it's because there's a breaking change.

If this is not the behaviour you want it should be possible to override it.

If there's a good case for reverting this I'm happy to have that discussion. If so feel free to re-open this issue.

@asgerhallas
Copy link
Author

This makes sense, thanks! I don't have a good case for reverting, that can not be handled another way 😊

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

No branches or pull requests

2 participants