Skip to content

Enforce comments on the codebase #2428

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

Closed
sedefsavas opened this issue May 25, 2021 · 12 comments · Fixed by #2443
Closed

Enforce comments on the codebase #2428

sedefsavas opened this issue May 25, 2021 · 12 comments · Fixed by #2443
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/backlog Higher priority than priority/awaiting-more-evidence.
Milestone

Comments

@sedefsavas
Copy link
Contributor

sedefsavas commented May 25, 2021

Enable linters for enforcing adding a comment to exported methods.

Relevant cluster-api PRs:
kubernetes-sigs/cluster-api#4631
kubernetes-sigs/cluster-api#4669

/milestone Next
/priority backlog
/good-first-issue
/help

@k8s-ci-robot
Copy link
Contributor

@sedefsavas:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Enable linters for enforcing adding a comment to exported methods.

Similar to this: kubernetes-sigs/cluster-api#4669

/milestone Next
/priority backlog
/good-first-issue
/help

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 k8s-ci-robot added this to the Next milestone May 25, 2021
@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels May 25, 2021
@Algebra8
Copy link
Contributor

Hello 👋

So it seems like kubernetes-sigs's cluster-api uses golint, so it should be as simple as adding that to .golangci.yml.

Should certain files, packages, or code also be excluded, e.g. (func|type).*Fake.*, .*test/ etc? (examples taken from the issue posted).

@sedefsavas
Copy link
Contributor Author

Hello @Algebra8!

Yes, wherever we don't need to add comments, we should exclude, e.g.,: *test, *fake, *mock, hack/.

After enabling this in golangci.yml, missing comments should be added as well, so expecting this to be a big PR.

@Algebra8
Copy link
Contributor

Ah ok, thanks for the heads up! I will work on my fork and make a PR when I think it's ready. Just a heads up though, I am currently interviewing so I won't be able to put 100% of my time and get it done ASAP. Is that alright?

@sedefsavas
Copy link
Contributor Author

Yes, we are 1 month away from the release. You can do a draft PR or WIP PR if you want feedback too.

/assign Algebra8

@Algebra8
Copy link
Contributor

Great, thanks @sedefsavas !

@devopsy-ir
Copy link

/assign

@sedefsavas
Copy link
Contributor Author

@mohsen-abbasi-shia Hi!
This issue was already assigned to @Algebra8, so unassigning you. We have other unassigned good-first-issues you can pick up.
/unassign @mohsen-abbasi-shia

@sedefsavas
Copy link
Contributor Author

This is also relevant.
kubernetes-sigs/cluster-api#4631

@Algebra8
Copy link
Contributor

This is also relevant.
kubernetes-sigs/cluster-api#4631

@sedefsavas Thanks for the resource! I'll make sure to include package X implements X comment requirements as well.

PS I think I'll be submitting a draft soon because there are a few things I would appreciate feedback/clarity on. Will be submitting the draft soon.

@Algebra8
Copy link
Contributor

Also, just a quick update: It seems like kubernetes-sigs/cluster-api@0fb33bc swapped using golint for revive. I believe this is because golint has been archived and golangci-lint recommends swapping it with revive. So, I'll be using revive instead of golint as well, unless asked not to.

@Algebra8
Copy link
Contributor

Algebra8 commented Jun 8, 2021

@sedefsavas

Hope you're doing well. I have a few questions that I would appreciate feedback on here.

I would appreciate if you could provide some feedback or point me in the direction of the whom I should be getting feedback from.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants