You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "debae (via GitHub)" <gi...@apache.org> on 2023/03/26 15:15:29 UTC
[GitHub] [commons-lang] debae opened a new pull request, #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
debae opened a new pull request, #838:
URL: https://github.com/apache/commons-lang/pull/838
Change for LANG-1677 to make it possible to exclude fields in ReflectionDiffBuilder. Similar solution like the ReflectionToStringBuilder has been applied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on pull request #838: [LANG-1677] Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #838:
URL: https://github.com/apache/commons-lang/pull/838#issuecomment-1575668547
https://issues.apache.org/jira/browse/LANG-1667
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #838:
URL: https://github.com/apache/commons-lang/pull/838#issuecomment-1249681336
I'll look over the weekend.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r842029947
##########
src/changes/changes.xml:
##########
@@ -66,6 +66,7 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" dev="ggregory" due-to="CiprianBodnarescu">Use Set instead of List for checking the contains() method #734.</action>
<action type="fix" dev="kinow" due-to="Roland Kreuzer">Javadoc for StringUtils.substringBefore(String str, int separator) doesn't mention that the separator is an int.</action>
<!-- ADD -->
+ <action issue="LANG-1677" type="add" dev="debae" due-to="Dennis Baerten">Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude annotation to support field exclusion from diff.</action>
Review Comment:
Don't edit this file, it will usually clash depending on parallel changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r843999518
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -99,6 +107,34 @@ public ReflectionDiffBuilder(final T lhs, final T rhs, final ToStringStyle style
diffBuilder = new DiffBuilder<>(lhs, rhs, style);
}
+ /**
+ * Returns the field names that should be excluded from the diff
+ * @return Returns the excludeFieldNames.
+ * @since 3.13.0
+ */
+ public String[] getExcludeFieldNames() {
+ return this.excludeFieldNames.clone();
+ }
+
+
+ /**
+ * Sets the field names to exclude.
+ *
+ * @param excludeFieldNamesParam
+ * The field names to exclude from the diff or {@code null}.
+ * @return {@code this}
+ * @since 3.13.0
+ */
+ public ReflectionDiffBuilder setExcludeFieldNames(final String... excludeFieldNamesParam) {
+ if (excludeFieldNamesParam == null) {
+ this.excludeFieldNames = new String[0];
Review Comment:
Reuse `ArrayUtils.EMPTY_STRING_ARRAY`
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -73,6 +75,12 @@
private final Object right;
private final DiffBuilder<T> diffBuilder;
+ /**
+ * Which field names to exclude from output. Intended for fields like {@code "password"} or {@code "lastModificationDate}.
+ * @since 3.13.0
+ */
+ protected String[] excludeFieldNames;
Review Comment:
Make `private`.
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -99,6 +107,34 @@ public ReflectionDiffBuilder(final T lhs, final T rhs, final ToStringStyle style
diffBuilder = new DiffBuilder<>(lhs, rhs, style);
}
+ /**
+ * Returns the field names that should be excluded from the diff
+ * @return Returns the excludeFieldNames.
+ * @since 3.13.0
+ */
+ public String[] getExcludeFieldNames() {
+ return this.excludeFieldNames.clone();
+ }
+
+
+ /**
+ * Sets the field names to exclude.
+ *
+ * @param excludeFieldNamesParam
+ * The field names to exclude from the diff or {@code null}.
+ * @return {@code this}
+ * @since 3.13.0
+ */
+ public ReflectionDiffBuilder setExcludeFieldNames(final String... excludeFieldNamesParam) {
+ if (excludeFieldNamesParam == null) {
+ this.excludeFieldNames = new String[0];
+ } else {
+ //clone and remove nulls
Review Comment:
Space after `//`
##########
src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java:
##########
@@ -48,10 +48,14 @@
private final Object[] objectArrayField = {null};
private static int staticField;
private transient String transientField;
+ @DiffExclude
+ private String annotatedField = "a";
+ private String excludedField = "a";
Review Comment:
You should test a non-transient null field.
##########
src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java:
##########
@@ -92,7 +96,7 @@ public void test_primitive_difference() {
@Test
public void test_array_difference() {
final TypeTestClass firstObject = new TypeTestClass();
- firstObject.charArrayField = new char[] { 'c' };
+ firstObject.charArrayField = new char[]{'c'};
Review Comment:
Leave formatting alone here.
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -131,7 +167,15 @@ private boolean accept(final Field field) {
if (Modifier.isTransient(field.getModifiers())) {
return false;
}
- return !Modifier.isStatic(field.getModifiers());
+ if (Modifier.isStatic(field.getModifiers())) {
+ return false;
+ }
+ if (this.excludeFieldNames != null
Review Comment:
If you make the default value the empty array and the setter does not allow excludeFieldNames to be null, then you do not need this check? It might be better this way though, not sure.
##########
src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java:
##########
@@ -128,4 +132,43 @@ public void test_difference_in_inherited_field() {
final DiffResult list = firstObject.diff(secondObject);
assertEquals(1, list.getNumberOfDiffs());
}
+
+ @Test
+ public void test_no_differences_excluded_field() {
+ final TypeTestClass firstObject = new TypeTestClass();
+ firstObject.excludedField = "b";
+ final TypeTestClass secondObject = new TypeTestClass();
+
+ final DiffResult list = firstObject.diff(secondObject);
+ assertEquals(0, list.getNumberOfDiffs());
+ }
+
+ @Test
+ public void test_no_differences_diff_exclude_annotated_field() {
+ final TypeTestClass firstObject = new TypeTestClass();
+ firstObject.annotatedField = "b";
+ final TypeTestClass secondObject = new TypeTestClass();
+
+ final DiffResult list = firstObject.diff(secondObject);
+ assertEquals(0, list.getNumberOfDiffs());
+ }
+
+ @Test
+ public void testGetExcludeFieldNamesWithNullExcludedFieldNames() {
+ ReflectionDiffBuilder<TypeTestClass> reflectionDiffBuilder = new ReflectionDiffBuilder<>(new TypeTestClass(), new TypeTestChildClass(), SHORT_STYLE);
+ reflectionDiffBuilder.setExcludeFieldNames(null);
+ String[] excludeFieldNames = reflectionDiffBuilder.getExcludeFieldNames();
+ assertNotNull(excludeFieldNames);
+ assertEquals(0, excludeFieldNames.length);
+ }
+
+ @Test
+ public void testGetExcludeFieldNamesWithNullValuesInExcludedFieldNames() {
+ ReflectionDiffBuilder<TypeTestClass> reflectionDiffBuilder = new ReflectionDiffBuilder<>(new TypeTestClass(), new TypeTestChildClass(), SHORT_STYLE);
Review Comment:
Use final where you can.
##########
src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java:
##########
@@ -128,4 +132,43 @@ public void test_difference_in_inherited_field() {
final DiffResult list = firstObject.diff(secondObject);
assertEquals(1, list.getNumberOfDiffs());
}
+
+ @Test
+ public void test_no_differences_excluded_field() {
+ final TypeTestClass firstObject = new TypeTestClass();
+ firstObject.excludedField = "b";
+ final TypeTestClass secondObject = new TypeTestClass();
+
+ final DiffResult list = firstObject.diff(secondObject);
+ assertEquals(0, list.getNumberOfDiffs());
+ }
+
+ @Test
+ public void test_no_differences_diff_exclude_annotated_field() {
+ final TypeTestClass firstObject = new TypeTestClass();
+ firstObject.annotatedField = "b";
+ final TypeTestClass secondObject = new TypeTestClass();
+
+ final DiffResult list = firstObject.diff(secondObject);
+ assertEquals(0, list.getNumberOfDiffs());
+ }
+
+ @Test
+ public void testGetExcludeFieldNamesWithNullExcludedFieldNames() {
+ ReflectionDiffBuilder<TypeTestClass> reflectionDiffBuilder = new ReflectionDiffBuilder<>(new TypeTestClass(), new TypeTestChildClass(), SHORT_STYLE);
Review Comment:
Use final where you can.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
debae commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r842074232
##########
src/changes/changes.xml:
##########
@@ -66,6 +66,7 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" dev="ggregory" due-to="CiprianBodnarescu">Use Set instead of List for checking the contains() method #734.</action>
<action type="fix" dev="kinow" due-to="Roland Kreuzer">Javadoc for StringUtils.substringBefore(String str, int separator) doesn't mention that the separator is an int.</action>
<!-- ADD -->
+ <action issue="LANG-1677" type="add" dev="debae" due-to="Dennis Baerten">Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude annotation to support field exclusion from diff.</action>
Review Comment:
Removed the file again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae commented on pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
debae commented on PR #838:
URL: https://github.com/apache/commons-lang/pull/838#issuecomment-1320414172
@garydgregory did you have the time too look into this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
debae commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r844287142
##########
src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java:
##########
@@ -48,10 +48,14 @@
private final Object[] objectArrayField = {null};
private static int staticField;
private transient String transientField;
+ @DiffExclude
+ private String annotatedField = "a";
+ private String excludedField = "a";
Review Comment:
There is already a testcase `test_transient_field_difference` that validates this, there is no specific or new logic for transient fields that get excluded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "debae (via GitHub)" <gi...@apache.org>.
debae commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r1146888283
##########
src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java:
##########
@@ -128,4 +133,44 @@ public void test_difference_in_inherited_field() {
final DiffResult list = firstObject.diff(secondObject);
assertEquals(1, list.getNumberOfDiffs());
}
+
+ @Test
+ public void test_no_differences_excluded_field() {
+ final TypeTestClass firstObject = new TypeTestClass();
+ firstObject.excludedField = "b";
+ final TypeTestClass secondObject = new TypeTestClass();
+
+ final DiffResult list = firstObject.diff(secondObject);
+ assertEquals(0, list.getNumberOfDiffs());
+ }
+
+ @Test
+ public void test_no_differences_diff_exclude_annotated_field() {
+ final TypeTestClass firstObject = new TypeTestClass();
+ firstObject.annotatedField = "b";
+ final TypeTestClass secondObject = new TypeTestClass();
+
+ final DiffResult list = firstObject.diff(secondObject);
+ assertEquals(0, list.getNumberOfDiffs());
+ }
Review Comment:
Extra testcase added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory merged pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #838:
URL: https://github.com/apache/commons-lang/pull/838
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] kinow commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "kinow (via GitHub)" <gi...@apache.org>.
kinow commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r1103699647
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -131,7 +167,15 @@ private boolean accept(final Field field) {
if (Modifier.isTransient(field.getModifiers())) {
return false;
}
- return !Modifier.isStatic(field.getModifiers());
+ if (Modifier.isStatic(field.getModifiers())) {
+ return false;
+ }
+ if (this.excludeFieldNames != null
Review Comment:
>In case you do call setExcludeFieldNames with null, it's still safe here, guess that's the main reason.
But this if condition is still unreachable even after calling setExcludeFieldNames with null, so we can drop it too (as condition cannot be reached for users nor via test code, I think?).
##########
src/test/java/org/apache/commons/lang3/builder/ReflectionDiffBuilderTest.java:
##########
@@ -128,4 +133,44 @@ public void test_difference_in_inherited_field() {
final DiffResult list = firstObject.diff(secondObject);
assertEquals(1, list.getNumberOfDiffs());
}
+
+ @Test
+ public void test_no_differences_excluded_field() {
+ final TypeTestClass firstObject = new TypeTestClass();
+ firstObject.excludedField = "b";
+ final TypeTestClass secondObject = new TypeTestClass();
+
+ final DiffResult list = firstObject.diff(secondObject);
+ assertEquals(0, list.getNumberOfDiffs());
+ }
+
+ @Test
+ public void test_no_differences_diff_exclude_annotated_field() {
+ final TypeTestClass firstObject = new TypeTestClass();
+ firstObject.annotatedField = "b";
+ final TypeTestClass secondObject = new TypeTestClass();
+
+ final DiffResult list = firstObject.diff(secondObject);
+ assertEquals(0, list.getNumberOfDiffs());
+ }
Review Comment:
Maybe add a test that does both `annotatedField` and `excludedField` being excluded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] garydgregory commented on pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #838:
URL: https://github.com/apache/commons-lang/pull/838#issuecomment-1320420683
I will be busy this weekend with Commons BCEL and Commons Net, so probably after that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae commented on pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
debae commented on PR #838:
URL: https://github.com/apache/commons-lang/pull/838#issuecomment-1249516337
@garydgregory friendly reminder on this PR , are requested changes are tackled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] codecov-commenter commented on pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #838:
URL: https://github.com/apache/commons-lang/pull/838#issuecomment-1484150276
## [Codecov](https://codecov.io/gh/apache/commons-lang/pull/838?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#838](https://codecov.io/gh/apache/commons-lang/pull/838?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d851547) into [master](https://codecov.io/gh/apache/commons-lang/commit/ccb4ad2234dbc56a6396e4b1d54a1675b5446c5b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ccb4ad2) will **decrease** coverage by `0.01%`.
> The diff coverage is `90.90%`.
```diff
@@ Coverage Diff @@
## master #838 +/- ##
============================================
- Coverage 92.06% 92.06% -0.01%
- Complexity 7482 7487 +5
============================================
Files 194 194
Lines 15721 15731 +10
Branches 2897 2900 +3
============================================
+ Hits 14474 14483 +9
Misses 674 674
- Partials 573 574 +1
```
| [Impacted Files](https://codecov.io/gh/apache/commons-lang/pull/838?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...e/commons/lang3/builder/ReflectionDiffBuilder.java](https://codecov.io/gh/apache/commons-lang/pull/838?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvbGFuZzMvYnVpbGRlci9SZWZsZWN0aW9uRGlmZkJ1aWxkZXIuamF2YQ==) | `90.62% <90.90%> (-0.29%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae commented on pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "debae (via GitHub)" <gi...@apache.org>.
debae commented on PR #838:
URL: https://github.com/apache/commons-lang/pull/838#issuecomment-1527014772
@kinow and @garydgregory Can you guys have another look at this PR, all remarks have been processed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "debae (via GitHub)" <gi...@apache.org>.
debae commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r1146879020
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -131,7 +167,15 @@ private boolean accept(final Field field) {
if (Modifier.isTransient(field.getModifiers())) {
return false;
}
- return !Modifier.isStatic(field.getModifiers());
+ if (Modifier.isStatic(field.getModifiers())) {
+ return false;
+ }
+ if (this.excludeFieldNames != null
Review Comment:
If you do invoke the method setExcludeFieldNames() it will have a default value of null, and in that cause a NPE here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by GitBox <gi...@apache.org>.
debae commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r844273828
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -131,7 +167,15 @@ private boolean accept(final Field field) {
if (Modifier.isTransient(field.getModifiers())) {
return false;
}
- return !Modifier.isStatic(field.getModifiers());
+ if (Modifier.isStatic(field.getModifiers())) {
+ return false;
+ }
+ if (this.excludeFieldNames != null
Review Comment:
I took some "inspiration" from the `ReflectionToStringBuilder` where we also have the null check.
In case you do call setExcludeFieldNames with null, it's still safe here, guess that's the main reason.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] debae closed pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "debae (via GitHub)" <gi...@apache.org>.
debae closed pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
URL: https://github.com/apache/commons-lang/pull/838
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-lang] kinow commented on a diff in pull request #838: LANG-1677 : Add ReflectionDiffBuilder.setExcludeFieldNames(...) and DiffExclude a…
Posted by "kinow (via GitHub)" <gi...@apache.org>.
kinow commented on code in PR #838:
URL: https://github.com/apache/commons-lang/pull/838#discussion_r1160238001
##########
src/main/java/org/apache/commons/lang3/builder/ReflectionDiffBuilder.java:
##########
@@ -131,7 +167,15 @@ private boolean accept(final Field field) {
if (Modifier.isTransient(field.getModifiers())) {
return false;
}
- return !Modifier.isStatic(field.getModifiers());
+ if (Modifier.isStatic(field.getModifiers())) {
+ return false;
+ }
+ if (this.excludeFieldNames != null
Review Comment:
Hmm, I think I didn't understand the whole discussion here.
@garydgregory pointed that instead of having a default of `null`, we could have something like `private String[] excludeFieldNames = ArrayUtils.EMPTY_STRING_ARRAY;`
>If you do invoke the method setExcludeFieldNames() it will have a default value of null, and in that cause a NPE here.
That sounds incorrect, I think (it's late, so I could be wrong). If you call `setExcludeFieldNames`, there's a check if the value passed is `null`, and `this.excludeFieldNames = ArrayUtils.EMPTY_STRING_ARRAY;`. Thus, `excludeFieldNames` cannot be `null`.
So if you did as @garydgregory suggested, and if we keep the defensive check for `null` in `setExcludeFieldNames`, then I believe what @garydgregory initially suggested holds here?
>If you make the default value the empty array and the setter does not allow excludeFieldNames to be null, then you do not need this check? It might be better this way though, not sure.
Then, I think the question would be whether it's worth having the `binarySearch` knowing it's an empty array (maybe we could skip if if we know it's an empty array).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org