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

tools: Move to a submodule #78

Merged
merged 1 commit into from
Sep 11, 2020
Merged

tools: Move to a submodule #78

merged 1 commit into from
Sep 11, 2020

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented Sep 11, 2020

Renaming tools.go to tools_test.go isn't enough. These constraints are
still carried over to consumers. Renaming only drops them from
go mod vendor.

This moves tools dependencies to a tools submodule which we will never
publish.

Ref uber-go/multierr#38

Renaming tools.go to tools_test.go isn't enough. These constraints are
still carried over to consumers. Renaming only drops them from
`go mod vendor`.

This moves tools dependencies to a `tools` submodule which we will never
publish.

Ref uber-go/multierr#38
@codecov
Copy link

codecov bot commented Sep 11, 2020

Codecov Report

Merging #78 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #78   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          239       239           
=========================================
  Hits           239       239           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ccfa79...b2ce9d4. Read the comment docs.

@@ -8,6 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed
- Disallow incorrect comparison of atomic values in a non-atomic way.

### Removed
- Remove dependency on `golang.org/x/{lint, tools}`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

or maybe just Moved out test dependencies (since it doesn't affect module resolution etc, just go mod download)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're not dropping all test dependencies, though. Only dev-time dependencies.

@abhinav abhinav merged commit 501585e into master Sep 11, 2020
@abhinav abhinav deleted the abg/tools-submodule branch September 11, 2020 18:24
abhinav added a commit to uber-go/automaxprocs that referenced this pull request Jan 29, 2021
Move lint dependencies to a tools submodule so that these are not
introduced as dependencies of the package.

In other projects, we experimented with renaming `tools.go` to
`tools_test.go` but that wasn't enough either. The tools submodule
appears to be the safest approach.

Supersedes #32
Refs uber-go/atomic#78
abhinav added a commit to uber-go/automaxprocs that referenced this pull request Jan 29, 2021
Move lint dependencies to a tools submodule so that these are not
introduced as dependencies of the package.

In other projects, we experimented with renaming `tools.go` to
`tools_test.go` but that wasn't enough either. The tools submodule
appears to be the safest approach.

Supersedes #32
Refs uber-go/atomic#78
abhinav added a commit to uber-go/automaxprocs that referenced this pull request Feb 1, 2021
Move lint dependencies to a tools submodule so that these are not
introduced as dependencies of the package.

In other projects, we experimented with renaming `tools.go` to
`tools_test.go` but that wasn't enough either. The tools submodule
appears to be the safest approach.

Supersedes #32
Refs uber-go/atomic#78
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.

2 participants