-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Remove gocyclo #2211
Comments
Sure if we're trying to optimize for review ease - then I agree. Also just from my own experience the majority (although not all) times I've had go-cyclo errors, it seems like it's unnecessary and I end up just using I agree, let's remove pre-launch and then maybe add back in post-launch at some point (maybe using a our own fork) |
We can investigate re-introducing our own fork #postlaunch. Closes #2211
@ValarDragon I agree as long as we don't have 300 lines functions. Gocyclo is useful specially for contributors that may not be very familiar with our coding guidelines, so will have ask to keep things simple in the contributor's PRs |
OK, fine for now. |
Summary
I think gocyclo is doing more harm than good the way were currently using it. You add some functionality to a function in a PR your writing, but you added an extra conditional. This addition is a small part of a larger PR. But now gocyclo says the function is too complex, so you go to refactor it. However this means your mixing refactors and code changes, which makes it super hard to review by diff, and just increases the PR size.
I think this is a problem, as we need to optimize for ease of reviewing PR's. The above situation happened to me in #2187, which made the PR hard to review. I also remember this happening in several PR's of reviewed.
Proposal
Remove gocyclo for now. I think it still has utility, but we need to solve this problem. (Have gocyclo errors which its fine to fix in a separate refactor PR. ) I don't think we need to spend time prelaunch figuring out the best approach here, and so I motion we remove gocyclo until postlaunch, OR change the gocyclo stuff to warn not cause linting to fail.
For Admin Use
The text was updated successfully, but these errors were encountered: