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

New linter: flag unnecessary calls to Finish on gomock.Controllers in Go 1.14+ #3223

Closed
jamietanna opened this issue Sep 16, 2022 · 13 comments
Closed
Labels
enhancement New feature or improvement

Comments

@jamietanna
Copy link

Your feature request related to a problem? Please describe.

As highlighted in https://pkg.go.dev/github.com/golang/mock/gomock#NewController:

New in go1.14+, if you are passing a *testing.T into this function you no longer need to call ctrl.Finish() in your test methods.

Describe the solution you'd like.

A linting rule that can flag when we're unnecessarily calling .Finish, when using a Go version of 1.14+

Describe alternatives you've considered.

N/A

Additional context.

No response

@jamietanna jamietanna added the enhancement New feature or improvement label Sep 16, 2022
@ldez
Copy link
Member

ldez commented Sep 16, 2022

Hello,

FYI golangci-lint is a meta-linter, there are no internal linting rules.

@ldez ldez added the linter: idea idea of a linter label Sep 16, 2022
@jamietanna
Copy link
Author

That's fair! Are you happy me leaving this here in case someone else wants to pick it up and contribute it?

@ldez
Copy link
Member

ldez commented Sep 16, 2022

I flagged it as a linter idea, so yes we can keep it.
Can you improve the title of your issue?

@jamietanna jamietanna changed the title Flag unnecessary calls to Finish on gomock.Controllers in Go 1.14+ New linter: flag unnecessary calls to Finish on gomock.Controllers in Go 1.14+ Sep 16, 2022
@daikidev111
Copy link

Hi can I try implementing a linter based on this idea?

@jamietanna
Copy link
Author

Yes please, happy for you to go for it ☺

@daikidev111
Copy link

Sorry for a bit of delay as I was busy with my uni work. I have finished the implementation part and I will do more testing to ensure that it really works. Once done with it, I will throw a PR.

@Antonboom Antonboom added the good first issue Good for newcomers label Oct 1, 2023
@Antonboom
Copy link
Contributor

Antonboom commented Oct 1, 2023

@daikidev111, how is it going? :)

Pay attention that github.com/golang/mock is archived and moved under Uber support (github.com/uber-go/mock).

@daikidev111
Copy link

daikidev111 commented Oct 2, 2023

@Antonboom
I've completed 90% of the implementation. However, due to the package having different support now, I'm unsure whether I should finish the implementation and submit a pull request or not. Is it possible to create a linter for a package that isn't natively supported by Go?

@ldez
Copy link
Member

ldez commented Oct 2, 2023

Is it possible to create a linter for a package that isn't natively supported by Go?

It has never been supported natively by Go, it was related to a library.

The libraries inside the golang organization are not related to Go itself.

@daikidev111
Copy link

daikidev111 commented Oct 7, 2023

Thank you for the clarification. I will resume working on this!

@hendrywiranto
Copy link
Contributor

Hi, look like the interested user still doesn't move forward with this

so I created the linter here https://github.com/hendrywiranto/gomockcontrollerfinish
will open a PR soon to integrate it
also if you guys have other idea for the name I'm still open for it, maybe gomocklint might be a more suitable name

@alexandear
Copy link
Member

For the workaround see #4202 (comment)

@alexandear
Copy link
Member

@ldez maybe we should close this issue because it's possible to detect Finish calls with forbidigo?

@ldez ldez closed this as completed Nov 25, 2023
@ldez ldez added declined and removed good first issue Good for newcomers linter: idea idea of a linter declined labels Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants