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

WIP [LANG-1490] - Creates DiffResultView and DiffView classes #457

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0x6e69636f
Copy link
Contributor

@0x6e69636f 0x6e69636f commented Sep 13, 2019

Currently, a DiffResult and a Diff can only handle the differences of two objects of the same type A, and we cannot set custom differences for a DiffResult if encapsulated objects of type B are different.

I suggest to implement a DiffView class as a simple String field, String leftValue and String rightValue triplet.

I also wrote a DiffResultView class holding two objects of type T and a list of DiffView objects.

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

@0x6e69636f 0x6e69636f changed the title [LANG-1490] - Creates DiffResultView and DiffView classes WIP [LANG-1490] - Creates DiffResultView and DiffView classes Sep 13, 2019
@garydgregory
Copy link
Member

garydgregory commented Sep 13, 2019

Hi @NicolasBD ,

Both files are missing the Apache license header causing CI build failures: https://travis-ci.org/apache/commons-lang/builds/584638020?utm_source=github_status&utm_medium=notification

Gary

@0x6e69636f
Copy link
Contributor Author

0x6e69636f commented Sep 13, 2019

@garydgregory ok I'll add it, but what do you think about the idea itself ?

I'll add proper headers and tests once we decide if this should be added to the lib

@garydgregory
Copy link
Member

Let me take a closer look this weekend. In the meantime, I would also like to work on a patch that defines as Diffable:

public interface Diffable<T extends Diffable<T>>

This will allow diffs on a class and one of its subclasses for example.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Looks like a very interesting addition to [lang] @NicolasBD ! Had a quick read at the code, and it looks good! Thanks for the PR.

Few comments, mainly about the Checkstyle reports. It would be really helpful if you could rebase your branch, and then run mvn or just push to your branch and wait for the Travis build to validate the changes.

We also have some diff algorithms for text in Commons Text. I wonder if later the API's would converge or have something in common - https://github.com/apache/commons-text/tree/master/src/main/java/org/apache/commons/text/diff

Bruno

* @param rightValue holds the string representation of the value in the right side of the comparison.
*/
public DiffView(String field, String leftValue, String rightValue) {
this.field = field;
Copy link
Member

Choose a reason for hiding this comment

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

I think the files were formatted with tabs, and this may cause issues in Travis when it builds using Checkstyle.

@@ -0,0 +1,64 @@
package org.apache.commons.lang3.builder;
Copy link
Member

Choose a reason for hiding this comment

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

I think this file is missing the copyright license header.

.stream()
.map(DiffView::new)
.collect(Collectors.toList())
);
Copy link
Member

Choose a reason for hiding this comment

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

Probably will fail checkstyle due to tabs here, instead of spaces.

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