Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork201
support merging inline deltas split by whitespace or a minor equality (#168)#191
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Your PR looks good. Please change the default behaviour. And thx for using java-diff-utils.
| List<AbstractDelta<String>> inlineDeltas = DiffUtils.diff(origList, revList, equalizer).getDeltas(); | ||
| List<AbstractDelta<String>> originalInlineDeltas = DiffUtils.diff(origList, revList, equalizer) | ||
| .getDeltas(); | ||
| List<AbstractDelta<String>> inlineDeltas = inlineDeltaMerger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please change it so that the default configuration without a merger leaves the functionality as it was, so no default merger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for your review and feedback.
The default behaviour is not changed by my pull request. By default, theinlineDeltaMerger is set toDEFAULT_INLINE_DELTA_MERGER which does not change the deltas. It just returns the original deltas.
By using theDEFAULT_INLINE_DELTA_MERGER, we can omit having null-checks on theinlineDeltaMerger and the code part that you've quoted stays cleaner.
Mainly, I used this approach compared to a null-check as it seems to be more in line with the other mechanisms. E.g. having aDEFAULT_EQUALIZER instead of having if-else cascades where Object::equals is used in case theequalizer property is null.
Please advise how to continue. Shall I:
- leave my original code unchanged
- add a comment above your quoted code part that explains that the default-delta-merger does not merge any deltas
- Remove the
DEFAULT_INLINE_DELTA_MERGERand checking theinlineDeltaMergerfor null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@wumpz: Please advise how to continue.
Stale pull request message |
@wumpz: Is there anything I can do to help you decide on how to continue? |
b5b7dac intojava-diff-utils:masterUh oh!
There was an error while loading.Please reload this page.
I merged your changes. How did you test it? I assume you did not use the maven build. I had to change multiple methods that were not in Java 8. |
Thank you very much! |
However, thx for your contribution. |
No description provided.