Happy Linter!! #182
Happy Linter!! #182
Conversation
… function names as well.
|
lgtm |
There was a problem hiding this comment.
lgtm.
Had some edge case I ran into after dep ensure somehow with
ok github.com/kensipe/kudo/pkg/apis/kudo/v1alpha1 (cached)
? github.com/kensipe/kudo/cmd/manager [no test files]
? github.com/kensipe/kudo/pkg/apis [no test files]
? github.com/kensipe/kudo/pkg/apis/kudo [no test files]
? github.com/kensipe/kudo/pkg/client/clientset/versioned [no test files]
? github.com/kensipe/kudo/pkg/client/clientset/versioned/fake [no test files]
? github.com/kensipe/kudo/pkg/client/clientset/versioned/scheme [no test files]
? github.com/kensipe/kudo/pkg/client/clientset/versioned/typed/kudo/v1alpha1 [no test files]
? github.com/kensipe/kudo/pkg/client/clientset/versioned/typed/kudo/v1alpha1/fake [no test files]
# github.com/kensipe/kudo/pkg/kudoctl/cmd [github.com/kensipe/kudo/pkg/kudoctl/cmd.test]
pkg/kudoctl/cmd/install.go:30:17: undefined: install.CmdErrorProcessor
FAIL github.com/kensipe/kudo/pkg/kudoctl/cmd [build failed]
FAIL github.com/kensipe/kudo/pkg/kudoctl/cmd/install [build failed]
Process finished with exit code 2
# github.com/kensipe/kudo/pkg/kudoctl/cmd/install [github.com/kensipe/kudo/pkg/kudoctl/cmd/install.test]
pkg/kudoctl/cmd/install/install.go:86:55: undefined: "github.com/kensipe/kudo/vendor/github.com/kudobuilder/kudo/pkg/kudoctl/util/github".Client
pkg/kudoctl/cmd/install/install.go:86:75: undefined: kudo.Client
pkg/kudoctl/cmd/install/install.go:203:61: undefined: "github.com/kensipe/kudo/vendor/github.com/kudobuilder/kudo/pkg/kudoctl/util/github".Client
pkg/kudoctl/cmd/install/install.go:203:81: undefined: kudo.Client
pkg/kudoctl/cmd/install/install.go:218:68: undefined: "github.com/kensipe/kudo/vendor/github.com/kudobuilder/kudo/pkg/kudoctl/util/github".Client
pkg/kudoctl/cmd/install/install.go:218:88: undefined: kudo.Client
pkg/kudoctl/cmd/install/install.go:233:70: undefined: "github.com/kensipe/kudo/vendor/github.com/kudobuilder/kudo/pkg/kudoctl/util/github".Client
pkg/kudoctl/cmd/install/install.go:233:90: undefined: kudo.Client
? github.com/kensipe/kudo/pkg/client/informers/externalversions [no test files]
? github.com/kensipe/kudo/pkg/client/informers/externalversions/internalinterfaces [no test files]
? github.com/kensipe/kudo/pkg/client/informers/externalversions/kudo [no test files]
? github.com/kensipe/kudo/pkg/client/informers/externalversions/kudo/v1alpha1 [no test files]
? github.com/kensipe/kudo/pkg/client/listers/kudo/v1alpha1 [no test files]
? github.com/kensipe/kudo/pkg/controller [no test files]
so its maybe worth someone else trying to check on tests and repercussions from the name changes but other than that it looks good! 👍
| @@ -7,7 +7,7 @@ import ( | |||
|
|
|||
| func TestNewCmdInstallReturnsCmd(t *testing.T) { | |||
|
|
|||
| newCmdInstall := NewCmdInstall() | |||
| newCmdInstall := NewInstallCmd() | |||
There was a problem hiding this comment.
Glad you picked on it. After back n forth I decided to go with the Kubernetes approach, over the Cobra approach. But maybe I am missing some things here too.
My reasoning was:
The thing that bothered me here is that in upstream K8s it follows the pattern NewCmdActionname ( ref https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/apply/apply.go#L150 or https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/create/create.go#L95 ) while in Cobra it follows the pattern NewActionnameCmd ( ref https://github.com/spf13/cobra/blob/master/doc/cmd_test.go#L48 or https://github.com/spf13/cobra/blob/master/cobra/cmd/add.go#L32 ) - imo we should use Kubernetes tools/patterns where possible but outgrow as we see fit (similar with e.g. #166 (comment) ).
Well all said, thats just the reasoning behind why I settled for the Kubernetes approach in the first place. I see also inconsistencies with other projects on the horizon, such as https://github.com/dcos/dcos-cli/blob/915806a8cfd75a252f2d9f4cbece2b11b99d350c/pkg/cmd/plugin/plugin_list.go#L16 is using the NewCmdActionname pattern that I settled for too (which was btw also one reason that influenced my decision when originally looked at it).
There was a problem hiding this comment.
I was more looking for internal project consistency.. reading more below as it looks like I hit a hot topic!
| @@ -35,7 +35,7 @@ and serves as an API aggregation layer. | |||
| Version: version.Version, | |||
| } | |||
|
|
|||
| cmd.AddCommand(NewCmdInstall()) | |||
| cmd.AddCommand(NewInstallCmd()) | |||
| cmd.AddCommand(NewListCmd()) | |||
There was a problem hiding this comment.
the above is btw also the reason why you find the artifacts from very early in more the Cobra style than the Kubernetes style, which I wanted to refactor/name then once we start working on them ( thoughts of naming occured here as well #138 (comment) ).
| } | ||
|
|
||
| // GetMostRecentFrameworkContentDir returns the content of the most recent Framework | ||
| func (g *GithubClient) GetMostRecentFrameworkContentDir(framework string) (*github.RepositoryContent, error) { |
There was a problem hiding this comment.
Naming and consistency is one of the hardest things and the earlier we can enforce it and set good examples the merrier.
With that said, I agree on this change and everything that follows this overarching goal of consistency.
For the sake of it, allow me to explain my reasoning in the first place:
It was inspired by the Kubernetes project using e.g. func (c *nodes) ( ref https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/node.go#L132 or https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/extensions/v1beta1/replicaset.go#L70 ) which seemed uncommon in Go when making methods for the variable name to match the first letter of the struct the method is implemented on, so I was following here just upstream.
Alternatives are also imo gc for GithubClient so it makes it easier in the code when reading e.g. c.client.Repositories.GetContents. For the UX it would mean in my opinion that it is not immediate obvious to the reader what client is meant with it. E.g. gc defined in gc *github.Client is cleaner to me then. And we do it in other parts such https://github.com/kudobuilder/kudo/pull/182/files#diff-47795ca62796b678e9c01915094311ddR86 or k2c ( k2c *kudo.Client ), which means also if changing we should stay consistent in those other parts as well. Certainly something I want to caution to keep in the back of our minds.
There was a problem hiding this comment.
👍 Understand that reasoning. I trend towards more of the Go Style Guide versus the Kubernetes repository which traditionally doesn't necessarily follow best practices. Our repository will start to look more like good Go.
https://github.com/golang/go/wiki/CodeReviewComments#receiver-names
https://golang.org/doc/effective_go.html
The method receiver should still be c because the type is Client (which happens to be in the github package). But yeah, we should stay consistent.
Let's separately come up with a style guide we want to enforce. Those docs are two good canonical sources to start with.
There was a problem hiding this comment.
There was a problem hiding this comment.
This one's important too:
There was a problem hiding this comment.
we on the same page in our desires... a standard and consistency. I do like @fabianbaier reason for gc *github.Client but fall inline with general consensus and consistency. +1 on the style guide .
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The function passed as
RunEto Cobra took some digging into. Function name and dependent functions names were changed to reflect their purpose. It is the only non-linter change.Does this PR introduce a user-facing change?: