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

lint: Enable interfacer #2367

Closed
wants to merge 1 commit into from
Closed

lint: Enable interfacer #2367

wants to merge 1 commit into from

Conversation

siggy
Copy link
Member

@siggy siggy commented Feb 24, 2019

interfacer suggests narrower interface types:
https://github.com/mvdan/interfacer

Part of #217

Signed-off-by: Andrew Seigner siggy@buoyant.io

@siggy siggy self-assigned this Feb 24, 2019
@@ -11,6 +11,7 @@ linters:
- gosimple
- govet
- ineffassign
- interfacer
Copy link

Choose a reason for hiding this comment

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

https://github.com/mvdan/interfacer appears to be archived. should we find a currently maintained version of this? is the archived version still considered the recommended one?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah... we're limited to what golangci-lint provides. in some cases they maintain their own forks:
https://github.com/golangci/golangci-lint/blob/master/go.mod#L13-L29
...and in other cases they depend directly on other linters:
https://github.com/golangci/golangci-lint/blob/master/go.mod#L57

i think it's a worthwhile trade-off to defer to golangci-lint on linter selection, as it's fairly well-supported as a whole, and also quite fast compared to running each linter separately.

interfacer suggests narrower interface types:
https://github.com/mvdan/interfacer

Part of #217

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
@alpeb
Copy link
Member

alpeb commented Feb 27, 2019

Sometimes even if a type is more specific than necessary, it seems to me it can give more clarity, and even more type safety. Like with url *url.URL we're sure we're dealing with a URL, but the tool suggests url string, which can be anything.
Also something like client *http.Client sounds more conventional to me than client rest.HTTPClient.
So not sure if we should abide by this tool. Would love to hear what folks think though.

@rmars
Copy link

rmars commented Feb 27, 2019

@alpeb I had the same thoughts! esp regarding url.URL and string! I do feel like for some of the changes here, the unlinted type was more informative.

@siggy
Copy link
Member Author

siggy commented Feb 28, 2019

@alpeb @rmars This all sounds reasonable. I think for a lot of these linters, there are tradeoffs where the linter requires slightly less-intuitive code at the benefit (hopefully) of easier code review. After reading your comments, I agree the linter benefits do not outweigh the code changes required. I'll close this one. Thanks for the thoughtful feedback!

@siggy siggy closed this Feb 28, 2019
@siggy siggy deleted the siggy/interfacer-lint branch February 28, 2019 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants