Skip to content

ability to compare a bean or map or collection by the result of a method call #69

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

Merged
merged 4 commits into from
Jul 28, 2013

Conversation

j1ee
Copy link

@j1ee j1ee commented Jul 25, 2013

this allows us to configure if we want to compare a collection by the result of size() or if we want to call a specific getter on a bean.

unknown added 3 commits July 24, 2013 17:36
…ty. shown in MethodEqualExample. Had trouble getting spock and testng in my dev environment. will look further into this tomorrow to ensure not broken stuff and to get some coverage for this.
@@ -61,6 +61,17 @@ else if (nodeInspector.isEqualsOnly(collectionNode))
collectionNode.setState(Node.State.CHANGED);
}
}
else if (nodeInspector.isWithMethodEquals(collectionNode)){
Copy link
Owner

Choose a reason for hiding this comment

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

isWithMethodEquals sounds a little strange. How about hasAlternativeEqualsMethod?

Copy link
Author

Choose a reason for hiding this comment

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

It is checking equality of the result of the method, so
"isWithMethodResultEqual" is more explicit, if a little verbose.
Your call, I suppose!
On 25 Jul 2013 21:10, "Daniel Bechler" [email protected] wrote:

In src/main/java/de/danielbechler/diff/CollectionDiffer.java:

@@ -61,6 +61,17 @@ else if (nodeInspector.isEqualsOnly(collectionNode))
collectionNode.setState(Node.State.CHANGED);
}
}

  •   else if (nodeInspector.isWithMethodEquals(collectionNode)){
    

isWithMethodEquals sounds a little strange. How about
hasAlternativeEqualsMethod?


Reply to this email directly or view it on GitHubhttps://github.com//pull/69/files#r5408527
.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, yes, now I see. I'm still not sold on isWithMethodResultEqual though. Even with the context I had, I failed to see what it does by just reading the name. But I think we should rather look for a better name in the Configuration, as it will dictate the names in the remaining places.

Copy link
Author

Choose a reason for hiding this comment

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

I agree.
How about "isComparedByMethodResult"?
I think this is a little clearer. I will let you know if I think of
something better!
On 25 Jul 2013 21:38, "Daniel Bechler" [email protected] wrote:

In src/main/java/de/danielbechler/diff/CollectionDiffer.java:

@@ -61,6 +61,17 @@ else if (nodeInspector.isEqualsOnly(collectionNode))
collectionNode.setState(Node.State.CHANGED);
}
}

  •   else if (nodeInspector.isWithMethodEquals(collectionNode)){
    

Ah, yes, now I see. I'm still not sold on isWithMethodResultEqual though.
Even with the context I had, I failed to see what it does by just reading
the name. But I think we should rather look for a better name in the
Configuration, as it will dictate the names in the remaining places.


Reply to this email directly or view it on GitHubhttps://github.com//pull/69/files#r5409210
.

Copy link
Owner

Choose a reason for hiding this comment

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

Much better, but still not as descriptive, as I'd like. How about hasEqualsOnlyValueProviderMethod, given we're using equalsOnlyValueProviderMethod in the Configuration?

Copy link
Author

Choose a reason for hiding this comment

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

Yea cool, is more specific. Gotta stay consistent too. Sounds good.
On 25 Jul 2013 21:56, "Daniel Bechler" [email protected] wrote:

In src/main/java/de/danielbechler/diff/CollectionDiffer.java:

@@ -61,6 +61,17 @@ else if (nodeInspector.isEqualsOnly(collectionNode))
collectionNode.setState(Node.State.CHANGED);
}
}

  •   else if (nodeInspector.isWithMethodEquals(collectionNode)){
    

Much better, but still not as descriptive, as I'd like. How about
hasEqualsOnlyValueProviderMethod, given we're using
equalsOnlyValueProviderMethod in the Configuration?


Reply to this email directly or view it on GitHubhttps://github.com//pull/69/files#r5409765
.

@SQiShER
Copy link
Owner

SQiShER commented Jul 25, 2013

I like what I see! The only thing I have mixed feelings about is the name "methodEqual". It doesn't really express what it really is: a method returning an object that is used for an equals only check instead of the original object. But that's hard to put in a short name. I'd like to see equalsOnly in there, as that's a term already known from the configuration. It's not pretty, but what do you think of equalsOnlyValueProviderMethod?

SQiShER added a commit that referenced this pull request Jul 28, 2013
Added ability to compare a bean, map or collection by the result of a method call
@SQiShER SQiShER merged commit db25c53 into SQiShER:master Jul 28, 2013
@SQiShER
Copy link
Owner

SQiShER commented Jul 28, 2013

Great job! Much appreciated! I'll try to release a new version with your changes sometime this week.

@SQiShER
Copy link
Owner

SQiShER commented Jul 29, 2013

Github doesn't seem to recognize the email address you used for your commits. If you'd like to appear in the list of contributors, you should be able to add it somewhere in your accounts Settings.

@j1ee
Copy link
Author

j1ee commented Jul 29, 2013

I realised after. apparently github matches it up based on the email in
the commit itself (which I did not have configured).

It's fine though, don't worry about it.

On 29 July 2013 09:58, Daniel Bechler [email protected] wrote:

Github doesn't seem to recognize the email address you used for your
commits. If you'd like to appear in the list of contributors, you should be
able to add it somewhere in your accounts Settings.


Reply to this email directly or view it on GitHubhttps://github.com//pull/69#issuecomment-21707060
.

@SQiShER
Copy link
Owner

SQiShER commented Jul 29, 2013

I think you can even add it afterwards.

On 29.07.2013, at 11:15, j1ee [email protected] wrote:

I realised after. apparently github matches it up based on the email in
the commit itself (which I did not have configured).

It's fine though, don't worry about it.

On 29 July 2013 09:58, Daniel Bechler [email protected] wrote:

Github doesn't seem to recognize the email address you used for your
commits. If you'd like to appear in the list of contributors, you should be
able to add it somewhere in your accounts Settings.


Reply to this email directly or view it on GitHubhttps://github.com//pull/69#issuecomment-21707060
.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants