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

Added new Diff on both DiffBuilders for more detailed DiffModel. #83

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

Conversation

driekus77
Copy link

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!

Copy link
Owner

@mmanela mmanela left a 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;
Copy link
Owner

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?

Copy link
Author

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.

Copy link
Owner

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(
Copy link
Owner

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?

Copy link
Author

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,
Copy link
Owner

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

Copy link
Author

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(
Copy link
Owner

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?

Copy link
Author

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.

@ldsenow
Copy link

ldsenow commented Apr 3, 2022

Hi guys, i am wondering why we dont merge this PR? It is very useful for inline diff to have subpieces.

@mmanela
Copy link
Owner

mmanela commented Apr 3, 2022

There are unresolved comments I believe.

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

Successfully merging this pull request may close these issues.

3 participants