-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Hygiene: enable linters for error conventions. #6264
Conversation
/retest-required |
/test check-pr-has-kind-label |
@bendory: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks! There are lot of error naming changes in trusted resources. 😄
/test pull-tekton-pipeline-alpha-integration-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: for these linter PRs, do you have any sense of how well these plugins are being maintained, and whether the plugins represent generally accepted code style or just the opinions of the plugin author? I'm curious because I don't see anything wrong with SubcommandSuccessful
as a function name, but this linter seems to have a problem with it.
It is maintained well enough that it's included by Go as a standard module; it's been adopted by Google, RedHat OpenShift, AWS, IBM, FaceBook, Netflix, and other major companies. With regard to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, one "nit" about not only renaming the error but maybe aliasing and mark as deprecated the old ones 👼🏼
/retest-required |
2 similar comments
/retest-required |
/retest-required |
Test failure is happening at |
/retest-required |
There are no expected functional changes in this PR. Made non-functional code changes to adhere to linter standards. Context: #5899 See related style docs: - https://github.com/golang/go/wiki/Errors#naming - https://google.github.io/styleguide/go/guide#concision - https://go.dev/blog/errors-are-values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
🎉 🎉 🎉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick, wlynch, Yongxuanzhang 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 |
Changes
There are no expected functional changes in this PR.
Made non-functional code changes to adhere to linter standards.
Context: #5899
See related style docs:
/kind cleanup
Submitter Checklist
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes