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

Hide github.com/matryer/moq dependency in tools.go from importers #2287

Merged
merged 1 commit into from
Jul 15, 2022

Conversation

danielwhite
Copy link
Contributor

I have project(s) that use go mod vendor. Currently, github.com/matryer/moq is vendored despite it not being required at runtime.

Moving tools.go to internal hides this import from downstream users and avoids github.com/matryer/moq being vendored.

go generate of the mocks still works as expected.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

Projects that use `go mod vendor` will vendor `github.com/matryer/moq`
despite it not being required at runtime.

Moving `tools.go` to `internal` hides this import from downstream
users and avoids `github.com/matryer/moq` being vendored.

`go generate` of the mocks still works as expected.

The assumption behind the import test broke, so I've pointed it at a
different path that has no Go code. This seems to match the intent
behind the original test for the `internal/code/..` path.
@@ -40,6 +40,6 @@ func TestNameForDir(t *testing.T) {

assert.Equal(t, "tmp", NameForDir("/tmp"))
assert.Equal(t, "code", NameForDir(wd))
assert.Equal(t, "internal", NameForDir(wd+"/.."))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for failing to sanity check the tests initially.

Turns out that the assumption behind the this assertion broke due to moving tools.go. I've pointed it at a different path that has no Go code. This seems to match the intent behind the original test for the internal/code/.. path.

I may have misunderstood, so happy for alternative suggestions. 🤔

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 75.023% when pulling a4defc3 on danielwhite:hide-moq-dependency into 3049369 on 99designs:master.

@StevenACoffman StevenACoffman merged commit f0e9047 into 99designs:master Jul 15, 2022
@StevenACoffman
Copy link
Collaborator

Thanks!

@danielwhite danielwhite deleted the hide-moq-dependency branch July 15, 2022 17:16
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