Add TypedEqualsBuilder class and test #1114

Cousnouf · 2023-09-25T07:23:14Z

This class allows to have a more complete equals builder that takes in consideration the null and class comparison before the appended fields.
It makes the equals method more compact.

ecki · 2023-09-27T10:34:07Z

Change looks quite good and complete. Not sure about the size, it might require a Apache contributor agreement. With this PR and Dev mail you probably don’t need an extra JIRA (or a committer can do that).

Do you have an contributors agreement on file, by any chance? https://www.apache.org/licenses/contributor-agreements.html

garydgregory · 2023-09-27T11:35:17Z

Hello @Cousnouf
Please run mvn locally and fix issues before you push to avoid broken builds.

garydgregory

Needs discussion. See my comments. The main comment would be the: Why is this a new class? The class name and docs are confusing. I should not have to read a code example (thankfully in the Javadoc at least) to try to understand what this does. I may not be a native English speaker but this means nothing to me: "An extension of {@link EqualsBuilder} aimed to perform additionally the base objects and class equals checks."

garydgregory · 2023-09-27T11:36:54Z

src/main/java/org/apache/commons/lang3/builder/TypedEqualsBuilder.java

+import java.util.function.Function;
+
+/**
+ * An extension of {@link EqualsBuilder} aimed to perform additionally the base objects and class equals checks.


Documentation needs a rework. I can't understand what this means.

garydgregory · 2023-09-27T11:37:28Z

src/main/java/org/apache/commons/lang3/builder/TypedEqualsBuilder.java

+    }
+
+    @Override
+    boolean shouldLeaveEarly() {


Leave what early? This methods needs a better name IMO?

garydgregory · 2023-09-27T11:38:28Z

src/main/java/org/apache/commons/lang3/builder/TypedEqualsBuilder.java

+        return sameReference || super.shouldLeaveEarly();
+    }
+
+    public TypedEqualsBuilder<T> append(Function<T, ?> extractor) {


What if I my function throws an exception? Should this use FailableFunction instead?

If the function throws a checked exception it can use FailableFunction, yes. I'll catch the Exception class (Throwable are meant to not be caught)

garydgregory · 2023-09-27T11:41:07Z

src/main/java/org/apache/commons/lang3/builder/TypedEqualsBuilder.java

+ *
+ * @since 3.14.0
+ */
+public class TypedEqualsBuilder<T> extends EqualsBuilder {


This is a confusing class name IMO. What is and where is the Type? It's not a Java Type or Class apparently...

It is the class type. its refering to the T class parameter which is supported by this Builder subtype.

This equals builder is typed. We provide a type and then the field functions to build the equals method.
So the Type is a Java Class.

It is the class type. its refering to the T class parameter which is supported by this Builder subtype.

✅️ Thanks for pointing that out! 👍

elharo

I do not understand the point of this PR. Please file an issue in the JIRA and add documentation in this PR.

elharo · 2023-09-27T11:48:15Z

src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java

@@ -198,7 +198,7 @@ private static void unregister(final Object lhs, final Object rhs) {
     * If the fields tested are equals.


equals --> equal

elharo · 2023-09-27T11:48:37Z

src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java

@@ -450,6 +450,15 @@ public static boolean reflectionEquals(final Object lhs, final Object rhs, final
                    .isEquals();
    }

+    /**
+     * Indicates if an early append method leave should be done


unclear, rephrase

elharo · 2023-09-27T11:50:37Z

src/test/java/org/apache/commons/lang3/builder/TypedEqualsBuilderTest.java

+        assertNotEquals(testObject1, testObject2);
+        assertNotEquals(testObject2, testObject1);
+
+        assertEquals(Boolean.TRUE,


probably use assertTrue and assertFalse, unless perhaps there's some auto-boxing detail I'm missing

It was done to make the test slightly more quick to read with the syntax coloration.

In general, do not assume devs are using IDEs. E.g. right now I'm reading this code in Github, and it's less clear than assertTrue

GitHub has also syntax coloration. But yes let's change these asserts.

elharo · 2023-09-27T11:50:46Z

src/test/java/org/apache/commons/lang3/builder/TypedEqualsBuilderTest.java

+                        .isEquals());
+    }
+
+    static class TestObject {


elharo · 2023-09-27T13:03:17Z

src/test/java/org/apache/commons/lang3/builder/TypedEqualsBuilderTest.java

+        assertNotEquals(testObject1, testObject2);
+        assertNotEquals(testObject2, testObject1);
+
+        assertEquals(Boolean.TRUE,


In general, do not assume devs are using IDEs. E.g. right now I'm reading this code in Github, and it's less clear than assertTrue

Cousnouf · 2023-09-27T14:42:26Z

I've addressed the comments. This version may tend to something better :)

Add TypedEqualsBuilder class and test

Cousnouf · 2023-10-02T09:21:02Z

Hello all, I've added some changes to address your comments. It's still in a separate class as I think it's better to separate the concept of EqualsBuilder with concrete values passed each time with append and the concept of TypedEqualsBuilder which receive the two objects in the construction and then calls the failable function during each appending. Inputs welcome :) Regards, Marc Le jeu. 28 sept. 2023 à 15:39, Gary Gregory ***@***.***> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/apache/commons/lang3/builder/TypedEqualsBuilder.java <#1114 (comment)>: > + * <p>Typical use for the code is as follows:</p> + * <pre> + * public boolean equals(Object obj) { + * return new TypedEqualsBuilder<>(this) + * .appendBaseObject(obj) + * .append(TestObject::getA) + * .append(TestObject::getB) + * .isEquals(); + * } + * </pre> + * + * @param <T> the type of the compared object. + * + * @SInCE 3.14.0 + */ +public class TypedEqualsBuilder<T> extends EqualsBuilder { It is the class type. its refering to the T class parameter which is supported by this Builder subtype. ✅️ Thanks for pointing that out! 👍 — Reply to this email directly, view it on GitHub <#1114 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AATRSC2QF3DIIJVO5GMIKO3X4V4ZLANCNFSM6AAAAAA5FT7FAY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

elharo

This certainly requires a CLA
There needs to be a JIRA and a clear proposal both for what problem is being solved, and how this proposal solves it.
I might see what problem is being solved here — I'm not sure — but if so this is not the way to go about solving it and this is not the right API.

That is, please focus on documenting and getting agreement on the plan before writing any more code. As is, it's hard to fairly consider this since we don't understand it fully.

elharo · 2023-10-02T11:55:57Z

src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java

@@ -195,10 +195,10 @@ private static void unregister(final Object lhs, final Object rhs) {
    }

    /**
-     * If the fields tested are equals.
+     * If the fields tested are equal


missing punctuation, sentence fragment

elharo · 2023-10-02T11:56:24Z

src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java

     * The default value is {@code true}.
     */
-    private boolean isEquals = true;
+    protected boolean isEquals = true;


private please. Use getters/setters if needed

elharo · 2023-10-02T11:56:46Z

src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java

@@ -450,6 +450,15 @@ public static boolean reflectionEquals(final Object lhs, final Object rhs, final
                    .isEquals();
    }

+    /**
+     * Indicates if we should interrupt the comparison during an appending.


third person, or second person if you must. Tech docs should never be second person plural

elharo · 2023-10-02T11:57:14Z

src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java

+    protected boolean interruptEqualityCheck() {
+        return !isEquals;
+    }
+
    /**
     * Tests if two {@code objects} by using reflection.


sentence fragment

elharo · 2023-10-02T11:58:20Z

src/main/java/org/apache/commons/lang3/builder/TypedEqualsBuilder.java

+import java.util.function.Function;
+
+/**
+ * An extension of {@link EqualsBuilder} that is typed and meant to append field getter functions.


Needs a rewrite

garydgregory requested changes Sep 27, 2023

View reviewed changes

elharo suggested changes Sep 27, 2023

View reviewed changes

Cousnouf force-pushed the typed_equals_builder branch from daa8c6e to 602c964 Compare September 27, 2023 14:40

https://issues.apache.org/jira/browse/LANG-1712

d70d4c7

Add TypedEqualsBuilder class and test

Cousnouf force-pushed the typed_equals_builder branch from 602c964 to d70d4c7 Compare September 27, 2023 14:44

elharo suggested changes Oct 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TypedEqualsBuilder class and test #1114

Add TypedEqualsBuilder class and test #1114

Cousnouf commented Sep 25, 2023 •

edited

Loading

ecki commented Sep 27, 2023

garydgregory commented Sep 27, 2023

garydgregory left a comment

garydgregory Sep 27, 2023

garydgregory Sep 27, 2023

garydgregory Sep 27, 2023

Cousnouf Sep 27, 2023

garydgregory Sep 27, 2023

ecki Sep 27, 2023

Cousnouf Sep 27, 2023

garydgregory Sep 28, 2023

elharo left a comment

elharo Sep 27, 2023

elharo Sep 27, 2023

elharo Sep 27, 2023

Cousnouf Sep 27, 2023 •

edited

Loading

elharo Sep 27, 2023

Cousnouf Sep 27, 2023

elharo Sep 27, 2023

elharo Sep 27, 2023

Cousnouf commented Sep 27, 2023

Cousnouf commented Oct 2, 2023 via email

elharo left a comment

elharo Oct 2, 2023

elharo Oct 2, 2023

elharo Oct 2, 2023

elharo Oct 2, 2023

elharo Oct 2, 2023

		@@ -198,7 +198,7 @@ private static void unregister(final Object lhs, final Object rhs) {
		* If the fields tested are equals.

Add TypedEqualsBuilder class and test #1114

Are you sure you want to change the base?

Add TypedEqualsBuilder class and test #1114

Conversation

Cousnouf commented Sep 25, 2023 • edited Loading

ecki commented Sep 27, 2023

garydgregory commented Sep 27, 2023

garydgregory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cousnouf Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cousnouf commented Sep 27, 2023

Cousnouf commented Oct 2, 2023 via email

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cousnouf commented Sep 25, 2023 •

edited

Loading

Cousnouf Sep 27, 2023 •

edited

Loading