-
Notifications
You must be signed in to change notification settings - Fork 4.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
fix-pkg-staticcheck and remove the repeat code #8035
fix-pkg-staticcheck and remove the repeat code #8035
Conversation
/cc @justinsb |
I don't quite see the value of removing the unreferenced convenience functions. I think it would make sense to either consolidate the convenience functions to just the ones in |
@johngmyers Thanks your view. |
@rifelpet The Same Question |
/cc @mikesplain |
I'd say if there is a linter that is reliable that we can use, I am in favor of more static analysis to help us in code review. We've started using hack/verify-staticcheck.sh (albeit with a lot of excluded packages right now) While in general I agree with @johngmyers that if we have two functions i32 and i64, and we stop using i64, then we shouldn't necessarily rush to delete i64. But I do think that having static checkers helping us avoid mistakes is possibly more important. I think @johngmyers raises an excellent point though, in that we could also put these into a helper package. (Unfortunately So I'm in favor of this PR, in that if we can get /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, tanjunchen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
1:remove some repeat code
2:fix some staticcheck