opt(hgctl/dashboard): avoid printing error messages cannot open browser #665
Conversation
sjcsjc123
requested review from
SpecialYang,
johnlanni,
Charlie17Li and
Xunzhuo
as code owners
|
|
|
Why are there only a plain URL showing in the terminal? |
|
Is it okay to add and print this information? |
Could you post a screenshot of the final output? Thanks. |
|
Of course. |
pkg/cmd/hgctl/dashboard.go
Outdated
| func openCommand(command string, args ...string) error { | ||
| _, err := exec.LookPath(command) | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
Why don't we return err but a nil here?
There was a problem hiding this comment.
When the command cannot be found, it will return err. At this time, simply print a reminder and return nil. If it returns err, there will be a step to print err later.
There was a problem hiding this comment.
I think if a function cannot perform the task it claims to perform, an error should be returned. And if there is no error returned, how should we determine whether to show the corresponding message then?
There was a problem hiding this comment.
How about this? If the command cannot be found, it will prompt that the browser cannot be opened; Otherwise, return err.
There was a problem hiding this comment.
Which one is better?
- Display an error message in openCommand and return nil
- Return an error in openCommand function and show the corresponding error message in the caller
There was a problem hiding this comment.
I tend to print it out in the opencommand method and then change it to a method with no return parameters.
|
We already have a flag for it: |
By default, browser is set to true, and users may not notice this flag. Running the default command without a GUI interface will also print the error. This is just my suggestion. |
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Xunzhuo
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #665 +/- ##
==========================================
- Coverage 35.63% 35.60% -0.03%
==========================================
Files 73 73
Lines 10439 10447 +8
==========================================
Hits 3720 3720
- Misses 6432 6440 +8
Partials 287 287
|
Signed-off-by: sjcsjc123 <[email protected]>
Signed-off-by: sjcsjc123 <[email protected]>
Ⅰ. Describe what this PR did
Avoid printing error messages when commands do not exist.
Ⅱ. Does this pull request fix one issue?
fixes #652
Ⅲ. Why don't you add test cases (unit test/integration test)?
This is related to the presence or absence of a GUI.
Ⅳ. Describe how to verify it
This is executed in the centos container of Docker.
This is executed in local.
Ⅴ. Special notes for reviews