-
Notifications
You must be signed in to change notification settings - Fork 136
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
feat!: change describe
command to info
#474
feat!: change describe
command to info
#474
Conversation
The describe command conflicts sematically with kubectl describe. This commit changes the command name to `info`. Fixes: knative#337 Signed-off-by: Lance Ball <lball@redhat.com>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance 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 |
@@ -31,7 +31,7 @@ func NewDescriber(namespaceOverride string) (describer *Describer, err error) { | |||
// restricts to label-syntax, which is thus escaped. Therefore as a knative (kube) implementation | |||
// detal proper full names have to be escaped on the way in and unescaped on the way out. ex: | |||
// www.example-site.com -> www-example--site-com | |||
func (d *Describer) Describe(ctx context.Context, name string) (description fn.Description, err error) { | |||
func (d *Describer) Describe(ctx context.Context, name string) (description fn.Info, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (d *Describer) Describe(ctx context.Context, name string) (description fn.Info, err error) {
Shouldn't this be changed as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that and decided not to change it for a couple of reasons. I meant to put this in the PR description, but just forgot.
Infoer
is a terrible name! 🤣- The
Describer
in theknative
package is responsible for the verb "describe" which the client uses to surface the noun "info" and they don't need to necessarily evolve 1:1 😎
But now by the light of morning, those reasons seem pretty weak. Do you have any suggestions for naming? I don't particularly like this.
type Infoer struct {
// ...
}
func (i *Infoer) Info(ctx context.Context, name string) (description fn.Info, err error)
What do you think of this?
type Info struct {
// ...
}
func (i *Info) Fetch(ctx context.Context, name string) (description fn.Info, err error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, seems like a tradeoff either way. I agree that Describer
seems a lot nicer than Infoer
, and that it's fine if the CLI and client library aren't exactly mapped, if it means the client library is closer to what we think is "correct". I could go either way on it, but leaving it Describe fits for me: run the info
command to get a description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, seems like a tradeoff either way. I agree that Describer
seems a lot nicer than Infoer
, and that it's fine if the CLI and client library aren't exactly mapped, if it means the client library is closer to what we think is "correct". I could go either way on it, but leaving it Describer
making a Description
still fits I think: "run the info
command to get more information, including a description from the describer" seems like a well-formed sentence, which is a good litmus test. Not to mention, there is the possibility that info
may display more than just the description from the describer in the future...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to mention, there is the possibility that
info
may display more than just the description from the describer in the future...
Thanks for mentioning this. I avoided saying it because it's easy to fall down the flexibility hole.
/lgtm |
Changes
describe
command toinfo
/kind enhancement
Fixes: #337
The describe command conflicts sematically with kubectl describe.
Release Note
Docs
Documentation changes are included in this PR.