Skip to content
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

pkg: address golangci var-naming issues #17584

Merged
merged 1 commit into from
Mar 24, 2024

Conversation

ivanvc
Copy link
Member

@ivanvc ivanvc commented Mar 13, 2024

Addresses the var-naming issues in pkg.

  • Renames the grpc_testing module to grpctesting
  • Renames the exported GrpcRecorder struct to GRPCRecorder

Part of #17578

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@ivanvc ivanvc mentioned this pull request Mar 13, 2024
20 tasks
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Just one question about which approach we use to fix grpc_testing below.

@@ -50,7 +50,7 @@ import (
"go.etcd.io/etcd/client/pkg/v3/transport"
"go.etcd.io/etcd/client/pkg/v3/types"
clientv3 "go.etcd.io/etcd/client/v3"
"go.etcd.io/etcd/pkg/v3/grpc_testing"
grpctesting "go.etcd.io/etcd/pkg/v3/grpc_testing"
Copy link
Member

@jmhbnz jmhbnz Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider changing this at a lower level and rename etcd/pkg/grpc_testing to etcd/pkg/grpctesting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree to rename etcd/pkg/grpc_testing to etcd/pkg/grpctesting. It's only used by test, so it should be fine to rename it although it's public.

The package was introduced in #12667. I guess the naming just follows the package used by grpc-go: google.golang.org/grpc/test/grpc_testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rename the package. Reference to Go best practice: https://go.dev/blog/package-names, where revive complains about underscores in package names: https://github.com/mgechev/revive/blob/ef34f92/rule/var-naming.go#L66-L72. Unfortunately, revive doesn't provide a website with its rules.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Signed-off-by: Ivan Valdes <ivan@vald.es>
@ivanvc ivanvc force-pushed the address-pkg-var-naming-lint-rule branch from 83113be to d98ff0d Compare March 15, 2024 04:18
@ahrtr
Copy link
Member

ahrtr commented Mar 15, 2024

/ok-to-test

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahrtr
Copy link
Member

ahrtr commented Mar 24, 2024

@ivanvc Please rebase this PR since we merged lots of PRs recently.

cc @serathius

@serathius serathius merged commit 49fbcd0 into etcd-io:main Mar 24, 2024
40 checks passed
@ivanvc ivanvc deleted the address-pkg-var-naming-lint-rule branch March 25, 2024 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants