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

get rid of the module dependency on sergi/go-diff #23

Closed
mvdan opened this issue Feb 26, 2021 · 1 comment
Closed

get rid of the module dependency on sergi/go-diff #23

mvdan opened this issue Feb 26, 2021 · 1 comment

Comments

@mvdan
Copy link
Contributor

mvdan commented Feb 26, 2021

I understand that it's just used in a test, and that if a main package imports pkg/diff, it won't end up depending on sergi/go-diff at a package level.

However, it's still undesirable for pkg/diff to pull in such relatively heavy dependencies. Beyond go-diff itself, we also end up pulling testify and even a yaml library (!!!).

It's true that this will become less of a concern with lazy module loading. I don't think that's a satisfactory answer though, because it won't ship until at least 1.17 in six months, and people will keep using go 1.16 or earlier in their go.mod files for at least a year or two.

So, I think we should remove it. It seems to only be used in a single test, to show a diff when the unified diff tests fail. I get why we don't want to use our own code; if the diffs are broken, showing a broken diff in the output isn't good.

I see two reasonable options:

  1. Use the host's diff tool in that case, like what gofmt -d does.

  2. Just show the entire output as-is. The tests aren't large, and failures should be rare.

@mvdan
Copy link
Contributor Author

mvdan commented Feb 26, 2021

I also think that, until lazy module loading is widespread (in at least a year), we should really try to have zero module dependencies.

mvdan added a commit to mvdan/diff that referenced this issue Feb 26, 2021
sergi/go-diff is only used in one test, though having it as a module
dependency is still noteworthy. That will become less of a problem once
lazy module loading is widespread, but that's still at least one year
away.

Plus, we only used go-diff for a simple use case: to show a diff for a
failed diff test. We already print the got/want text, so a diff is
strictly speaking not necessary. Plus, if printing the entire text is
reasonable, it's not like the diff would matter in terms of printing
less text.

While at it, bump the go language version from 1.13 to 1.15, since Go
1.14.x and older are no longer maintained.

Fixes pkg#23.
@mvdan mvdan closed this as completed in 20ebb0f Feb 26, 2021
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

No branches or pull requests

1 participant