-
-
Notifications
You must be signed in to change notification settings - Fork 193
Added new Diff on both DiffBuilders for more detailed DiffModel. #83
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
base: master
Are you sure you want to change the base?
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.
I did a quick first pass
using System.Linq; | ||
|
||
namespace DiffPlex.DiffBuilder.Model | ||
{ | ||
public class DiffPaneModel | ||
{ | ||
public List<DiffPiece> Chunks => Lines; |
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.
Why make an alias like this?
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.
Because when Line chunker isn't used as first the current name "Lines" doesn't represent its contents.
But its just a small alias np for me to delete it.
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.
Sounds fine, can you put summary comments on both maybe to clear up any confusion?
@@ -77,6 +80,130 @@ public static DiffPaneModel Diff(IDiffer differ, string oldText, string newText, | |||
return model; | |||
} | |||
|
|||
public static DiffPaneModel Diff( |
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.
Instread of having this logic close to twice, could we instread just share the BuildDiffPieces
logic that SideBySide uses, extract it to a common place? Then let inline do one last step to flatten it?
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.
Yes you are probably right. Was trying to solve it twice now.
public static SideBySideDiffModel Diff( | ||
IDiffer differ, | ||
string oldText, string newText, | ||
List<IChunker> detailsPack, |
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.
I like the idea to give an array, but detailsPack name is too general. maybe called it chunkers or something
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.
Oka then Chunkers it is.
@@ -104,6 +104,59 @@ public static SideBySideDiffModel Diff(IDiffer differ, string oldText, string ne | |||
return model; | |||
} | |||
|
|||
public static SideBySideDiffModel Diff( |
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.
Couldn't we update the other methods in this file to use the new more general method here?
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.
Good point If unit tests will still pass I'll look into this.
Hi guys, i am wondering why we dont merge this PR? It is very useful for inline diff to have subpieces. |
There are unresolved comments I believe. |
See Issue #29
Added on both InlineDiffBuilder and SideBySideDiffBuilder possibility to add more details by providing a detailsPack of IChunker(s).
The InlineDiffBuilder on lowest level, is still missing the old / before char info. Didn't know where to put it: After or Under(SubPiece) the new / after char. Advice / help is welcome!
In order to not introduce any breaking changes in the API I introduced new Diff overload methods. These method(s) distinguish themselves by a detailsPack (Name oke?) that include the IChunkers to use. If null then default all 3 chunkers are used: Lines => Words => Characters.
Review comments are welcome!
Thanks for this great lib!