yudai / gojsondiff

Go JSON Diff
Other
535 stars 81 forks source link

formatter.v1 imports gojsondiff from master #20

Open gavv opened 7 years ago

gavv commented 7 years ago

Hi,

It seems that current master is incompatible with v1, and they should not be used together. However, formatter.v1 imports gojsondiff from master, which causes problems.

Here is an example of a problem import: https://github.com/yudai/gojsondiff/blob/master/formatter/ascii.go#L9.

This example compiles well:

package main

import (
    "github.com/yudai/gojsondiff"
    "github.com/yudai/gojsondiff/formatter"
)

func main() {
    var d gojsondiff.Diff
    c := formatter.AsciiFormatterConfig{}
    f := formatter.NewAsciiFormatter(nil, c)
    f.Format(d)
}

But this one fails to compile:

package main

import (
    "gopkg.in/yudai/gojsondiff.v1"
    "gopkg.in/yudai/gojsondiff.v1/formatter"
)

func main() {
    var d gojsondiff.Diff
    c := formatter.AsciiFormatterConfig{}
    f := formatter.NewAsciiFormatter(nil, c)
    f.Format(d)
}

With the following error:

# command-line-arguments
./gjd.go:14: cannot use d (type "gopkg.in/yudai/gojsondiff.v1".Diff)
    as type "github.com/yudai/gojsondiff".Diff in argument to f.Format: 
      "gopkg.in/yudai/gojsondiff.v1".Diff does not implement
          "github.com/yudai/gojsondiff".Diff (wrong type for Deltas method)
                have Deltas() []"gopkg.in/yudai/gojsondiff.v1".Delta
                want Deltas() []"github.com/yudai/gojsondiff".Delta
gavv commented 7 years ago

I'm not sure what is the best way to fix this. One approach I know is the following:

This is a bit tedious, but should work.

yudai commented 7 years ago

Thanks for the report.

It seems that gopkg.in doesn't handle sub-packages well. However, I don't feel like rewriting import paths is a good idea. I don't want to add something just relies on gopkg.in. I rather want users to simply pin a commit ID in their package managers such as glide.

@gaav you don't want to use a package manager for some reason?

I don't want to break your library again and again, so thinking what's the best way.

gavv commented 7 years ago

Vendoring is a good thing, but I think it should be done in applications, not in libraries.

If every library would vendor all its dependencies (recursively!), we'll get lots of duplicated dependencies. For example, gojsondiff vendors ginkgo and gomega which may be used by httpexpect users, and so will be duplicated.

I prefer the approach when libraries provide stable branches and rely on stable branches of their dependencies, and it's up to applications to vendor them all.

With this scheme, there is no duplication, and the application developer controls updates, which is very important. For example, if a security fix is released for a library foo, there is no need to wait when all libraries that use foo will update their vendored copy of foo. The application developer can just apply the fix immediately because (s)he controls the vendoring.

Actually, the approach of creating a stable branch for every major release is not new and is used for ages outside the Go ecosystem.

I understand the reasons to avoid hardcoding gopkg.in paths. However, if you want a stable branch in Go, you need to choose some way to change import paths. For example you can just create new repos, e.g. gojsondiff2, gojsondiff3, etc. The advantage of gopkg.in is that managing branches is easier than managing repos. After all, you already hardcode github.com paths, and gopkg.in is just another service.

That was some general thoughts about vendoring and versioning in Go. But well, I don't mind vendoring gojsondiff in httpexpect if you'll choose not to provide a stable branch. Anyway, I suggest to document gojsondiff versioning scheme in README.

yudai commented 7 years ago

Maybe it's better to move formatter to the main package? let me see...

gavv commented 7 years ago

Sorry, I was wrong in the assumption that indirect dependencies vendored multiple times are duplicated. Instead, Go package managers flatten all nested vendor directories and every package is used exactly once. Therefore, my arguments related to the package duplication don't apply.

However, employing vendoring in libraries is problematic with flattening too. Problems will arise when different libraries vendor different incompatible versions of the same package with the same import path. Actually it seems to be even stronger argument to avoid vendoring in libraries.