🌱 Enable godoc linter checks #4631
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @stmcginnis. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XL
| @@ -48,8 +48,6 @@ issues: | |||
| - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked | |||
| # The following are being worked on to remove their exclusion. This list should be reduced or go away all together over time. | |||
| # If it is decided they will not be addressed they should be moved above this comment. | |||
| - (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form) | |||
| - package comment should be of the form "Package (.+) ..." | |||
There was a problem hiding this comment.
Let's keep this exclusion? Seems a lot of comment are in the form of package X implements X, which isn't really super useful?
There was a problem hiding this comment.
I don't have strong opinions here.
Ideally we should get few lines explaining what each package is, but the linter just ensures a minimal comment exist, not that an actual explanation exists.
However, if we consider this as a starting point I'm ok with enabling this linter/this PR
There was a problem hiding this comment.
My worry is that we add these descriptions to satisfy the linter, and provide no information to our users.
There was a problem hiding this comment.
We could keep the regex for just packages, but I would actually like to keep this.
I agree most of the docstrings are not very helpful right now. But at least they have a basis to be improved upon. There are a couple cases where it actually did add some helpful information and caught where there were good godocs, but not formatted correctly to show up in IDE help because of where they were placed. And it would help make sure any new packages added at least force the authors to think about whether there is anything helpful to add to the godocs.
So bottom line, the added value is questionable, but it at least has potential for helping without much of a downside. Does that make sense to you @vincepri?
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
stmcginnis
force-pushed
the
enable-golinters
branch
from
064059a to
c86f259
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
k8s-ci-robot
added
the
approved
|
Need a rebase |
This removes another of our linter exclusions to enforce linting that will error if packages or exported types do not have godoc comments on them. Signed-off-by: Sean McGinnis <[email protected]>
This removes the linting exclusion to make sure package godoc strings are in the expected format of "Package x...". Signed-off-by: Sean McGinnis <[email protected]>
stmcginnis
force-pushed
the
enable-golinters
branch
from
c86f259 to
36dd674
Compare
k8s-ci-robot
removed
the
lgtm
|
/test pull-cluster-api-test-main Unrelated failures. |
|
Merge conflicts resolved and tests are happy again. Please take a look again when you have some time @vincepri - thanks! |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What this PR does / why we need it:
This is a continuation of the work being done to enable some of the excluded linters we use.
This change removes the exclusions related to enforcing godoc comments for packages and
functions, variables, and other things that are public. It makes they all have associated doc
strings and that any package comments are in the expected format of
Package x....For package docs, if a given package was less than five separate files the comments were just
added to the
packagestatement in the first file. If there were five or more, adoc.gofile wasadded to keep it separate and easy to find.
Which issue(s) this PR fixes:
Related #4622