search

grpc / grpc-go

The Go language implementation of gRPC. HTTP/2 based RPC
https://grpc.io
Apache License 2.0
20.87k stars 4.33k forks source link

Add godoc linter #7444

Open dfawley opened 1 month ago

dfawley commented 1 month ago

Since removing golint, we've had numerous occasions of missing docstrings. This seems like the consensus replacement for golint: https://github.com/mgechev/revive -- we should set it up to at least check the docstrings.

EDIT:

7575 fixes

7574 fixes

7552 fixes

7528 fixes

7577 #7580 fixes

purnesh42H commented 1 month ago

Before we enforce revive, we need to fix existing lint issues caught in #7472 as part of vet check.

Its fine to change several files in a single PR if they all fix one type of issue or are otherwise related. However, be reasonable and try to keep the PRs small enough to make it not take a long time to review and avoid merge conflicts.

janardhanvissa commented 1 month ago

@purnesh42H Please assign me this issue.

janardhanvissa commented 1 month ago

@purnesh42H Right now, In the tests (vet) I'm getting like don't use an underscore in package name https://revive.run/r#var-naming. It's not expecting underscore but by default as it's generating from the proto file like ("google.golang.org/grpc/reflection/test/grpc_testing_not_regenerate"). So, what need to do for further. Please suggest.

janardhanvissa commented 1 month ago

@purnesh42H Fixed some of the godoc-linter issues. Please review and let me know if any changes are required. Please find the PR below. https://github.com/grpc/grpc-go/pull/7501

janardhanvissa commented 1 month ago

@purnesh42H Fixed linter issues for superfluous-else, increment-decrement, indent-error-flow, var-declaration. Please review the changes.

https://github.com/janardhanvissa/grpc-go/pull/1

janardhanvissa commented 4 weeks ago

@purnesh42H Fixed below godoc linter issues. Please find the PRs below. Please let me know if any changes are required.

15 redefines-builtin-id
8 package-comments
8 empty-block
6 var-declaration
3 indent-error-flow
2 increment-decrement
1 superfluous-else

https://github.com/janardhanvissa/grpc-go/pull/1 https://github.com/janardhanvissa/grpc-go/pull/2 https://github.com/janardhanvissa/grpc-go/pull/3

purnesh42H commented 3 weeks ago

@purnesh42H Fixed linter issues for superfluous-else, increment-decrement, indent-error-flow, var-declaration. Please review the changes.

janardhanvissa#1

Thanks. LGTM. Please open the. PR in main repo.

janardhanvissa commented 3 weeks ago

@purnesh42H Fixed below godoc linter issues and raised a PR to main repo.

6 var-declaration 3 indent-error-flow 2 increment-decrement 1 superfluous-else

janardhanvissa commented 3 weeks ago

@purnesh42H Fixed below godoc revive linter issues and raised a PR to main repo. Please find the PR below.

https://github.com/grpc/grpc-go/pull/7532

janardhanvissa commented 3 weeks ago

@purnesh42H Fixed below godoc revive linter issues and raised a PR to main repo. Please find the PR below.

https://github.com/grpc/grpc-go/pull/7550

janardhanvissa commented 3 weeks ago

@purnesh42H Fixed below godoc revive linter issues and raised a PR to main repo. Please find the PR below.

https://github.com/grpc/grpc-go/pull/7552

janardhanvissa commented 2 weeks ago

@purnesh42H Facing issue in context-as-argument(context.Context) should be the first parameter. Please have a look at the readme file (reflection/test/grpc_testing_not_regenerate/README.md), as the testv3.go is generated from the older version of v3. So, once we are updating and tried to generate the proto and this testv3 we are facing undefined function or var in other dependencies. It points to some external dependency with testv3 so any change in testv3 will have an impact which is not in our scope. Because it's imported from another repo. Suggest what we need to do to resolve this issue. If we are going to regenerate the testv3.proto file we need to delete the testv3.go and then regenerate. Then it will impact other dependency files like dynamic.go

janardhanvissa commented 2 weeks ago

@purnesh42H Raised a separate PRs in forked branch. Please review the changes. Please find the PRs below.

https://github.com/janardhanvissa/grpc-go/pull/6 https://github.com/janardhanvissa/grpc-go/pull/7

arvindbr8 commented 1 week ago

7577 fixes:

546 unused-parameter (ignored)

and renamed the check again

cc: @purnesh42H

purnesh42H commented 1 week ago

@janardhanvissa can you do these since we include unused-parameter now

gcp/observability/logging_test.go:592:20: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:592:41: parameter 'in' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:653:28: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:653:49: parameter 'config' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:676:20: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:676:41: parameter 'in' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:790:28: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:790:49: parameter 'config' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:825:20: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:825:41: parameter 'in' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:828:20: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:828:41: parameter 'in' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:1128:28: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:1128:49: parameter 'config' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:1152:20: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:1152:41: parameter 'in' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:1252:28: parameter 'ctx' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
gcp/observability/logging_test.go:1252:49: parameter 'config' seems to be unused, consider removing or renaming it as _ https://revive.run/r#unused-parameter
janardhanvissa commented 1 week ago

@purnesh42H Fixed above mentioned unused parameter linter issues and pushed the changes.

arvindbr8 commented 1 week ago

Adding this here for context:

https://github.com/grpc/grpc-go/pull/7552#issuecomment-2327328750

Based on the discussion, we have sufficient cases where having an empty block is justifiable, so it's reasonable to include an "empty-block" check within the revive tool.