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

Add a utility function with filenames #4

Open
mvdan opened this issue Jul 9, 2018 · 4 comments
Open

Add a utility function with filenames #4

mvdan opened this issue Jul 9, 2018 · 4 comments
Labels
back burner on hold for now

Comments

@mvdan
Copy link
Contributor

mvdan commented Jul 9, 2018

That is, func Files(left, right string) DiffWriteable. It would read the contents of the files up-front and diff the lines as []string or [][]byte, while including the original filenames. It would also do things that are common when one diffs plaintext files, such as treating windows line endings like unix line endings, or ignoring a trailing newline unless it appears/disappears.

In the rare case that someone needs this to support streaming, they can just easily implement the interface themselves.

@mvdan
Copy link
Contributor Author

mvdan commented Jun 12, 2019

A better alternative would be an option or wrapper that allows adding context to a diff operation, such as filenames. For example, imagine if I could write:

ab := diff.Strings(a, b)
ab = diff.WithFilenames(ab, "before.txt", "after.txt")

Or using option func parameters:

ab := diff.Strings(a, b, diff.Names("before.txt", "after.txt"))

@josharian
Copy link
Contributor

What do you think about Writeable shrinking to just two methods:

type Writeable interface {
	// WriteATo writes the element a[idx] to w.
	WriteATo(w io.Writer, idx int) (int, error)
	// WriteBTo writes the element b[idx] to w.
	WriteBTo(w io.Writer, idx int) (int, error)
}

And then have WriteUnified take context arguments to provide names, as well as dates, times, time zones, and the other goo that can show up in diff output. Something like:

func (e EditScript) WriteUnified(w io.Writer, ab Writeable, ctx ...WriteContext) (int, error)

Where WriteContext is something like:

type WriteContext interface {
  private() // private ensures that only types in this package can implement WriteContext
}

func WithNames(a, b string) WriteContext() {
  return names{a, b}
}

type names struct {
  a, b string
}

And then WriteUnified can simply peer inside all WriteContexts provided, since it knows all possible types that can occur. Use would look something like this:

ab := diff.Strings(a, b)
n, err := diff.WriteUnified(w, ab, diff.WithNames("before.txt", "after.txt"), diff.WithTimes(atime, btime))

(If no write contexts were provided, we'd use the names a and b, and omit dates/times/etc.)

@mvdan
Copy link
Contributor Author

mvdan commented Aug 28, 2019

I don't like ctx WriteContext because I think everyone is far too used to ctx context.Context :)

Why not call these options or metadata, which is much more common?

Beyond that, I like making names optional and the interface smaller.

josharian added a commit that referenced this issue Aug 30, 2019
Use WriteOpts instead of methods to control names.

Rename Writeable to WriterTo.

Updates #4
Updates #8
@josharian
Copy link
Contributor

I implemented WriteOpts as discussed here, and renamed Writeable to WriterTo. I think this has helped. I'm going to leave this open for future discussion and work on the original idea, which was a utility for diffing files, including a bunch of niceties discussed in the OP.

@josharian josharian added the back burner on hold for now label Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back burner on hold for now
Projects
None yet
Development

No branches or pull requests

2 participants