-
Notifications
You must be signed in to change notification settings - Fork 87
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
Use .Script to describe command if .Command is empty. #494
Conversation
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.
nothing blocking.. there is some concern of use. initially I didn't realize the string created is just for display purposes. It may be worth adding that in the godoc for (c *Command) String() string {
The most concerning is the magic numbers. It seems worth adding a constant having a name that clarifies purpose / intent.
It also likely makes sense to clarify the godoc on summarize
It is all functional... as such I'll approve. let me know when you are done and I'll merge if you prefer
|
||
import "strings" | ||
|
||
func (c *Command) String() string { |
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.
This is an implementation of the stringer interface and curious if it's implementation is consist or confusing to stringer expectations. Since it is just returning a string, perhaps it is... however the summarized output makes it seem like it is different.
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.
It's actually this fmt.Stringer, not the stringer tool. Its docstring says:
String method, which defines the “native” format for that value
unfortunately without explaining what "native format" means. :-(
I'm adding a docstring that clarifies the purpose.
|
||
func (c *Command) String() string { | ||
if c.Command == "" && c.Script == "" { | ||
return "(invalid command with neither Command nor Script set)" |
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.
This seems like an error message but is part of the string. Are we trying to avoid err messages and error handling on the calling side? Are we going to run into case of uses where the caller is going to need to scan for these error messages?
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 don't intend the value returned by String
to be machine-readable. I'm just trying to make sure that if for some reason in the future an invalid Command
object is included in the kuttl
log, then its description will be useful to a human. I feel like such message does this, unlike a simple empty string.
FTR, neither this nor the next case should ever happen in real life, since we already reject Command
objects where .Command == "" XOR .Script == ""
isn't met during unmarshalling. But better safe than sorry.
lines = append(lines, line) | ||
} | ||
joined := strings.Join(lines, "\\n ") | ||
if len(joined) > 70 { |
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.
70
and 67
seem like magic numbers... it would be best to have constants or a constant that expresses a string limit. I would recommend a string limit constant and a (constant-3) or join[:SIZE_LIMIT-len("...") + "..."
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.
Done
return summarize(c.Script) | ||
} | ||
|
||
// summarize returns a short representation of a multi-line command. |
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.
this method doesn't just shorten a multi-line command... in fact it is somewhat js aware and removes comments and set conditions. It seems designed specific to scripts and not commands.
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.
Good point, docstring enhanced.
} | ||
lines = append(lines, line) | ||
} | ||
joined := strings.Join(lines, "\\n ") |
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.
joined := strings.Join(lines, "\\n ") | |
joined := strings.Join(lines, `\n `) |
seems better to remove escape chars
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.
Done
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
What this PR does / why we need it:
Introduce a
String()
method that summarizes a command paying attention to.Script
too, and use it in appropriate places.While at it, also provide a command summary when it exits non-zero. Before this change, the error was:
After this change, the last line will be a more helpful:
Fixes #493