Update coverage tool #105
Update coverage tool #105
Conversation
xhd2015
force-pushed
the
update-coverage-tool
branch
2 times, most recently
from
bceecf1 to
13df832
Compare
xhd2015
force-pushed
the
update-coverage-tool
branch
from
13df832 to
241ff81
Compare
| @@ -19,6 +19,10 @@ func Main(args []string) { | |||
| fmt.Print(strings.TrimPrefix(help, "\n")) | |||
| return | |||
| } | |||
| if cmd != "merge" && cmd != "compact" { | |||
There was a problem hiding this comment.
Using a switch here would be more visible, and make sense I think as you will keep adding commands
switch (cmd) {
case "help":
fmt.Print(strings.TrimPrefix(help, "\n"))
return
case "merge", "compact":
// everything is fine
default:
fmt.Fprintf(os.Stderr, "unrecognized cmd: %s\n", cmd)
os.Exit(1)
}There was a problem hiding this comment.
I'm not even sure this code is needed as there is another switch after parsing flags
There was a problem hiding this comment.
This help command detection here should/should be combined with flagHelp, BTW
| } | ||
| // if mode == "" { | ||
| // mode = covMode | ||
| // } |
There was a problem hiding this comment.
Was this code commented and forgotten? Or left behind?
I'm asking just in case, as it looks strange
There was a problem hiding this comment.
I forgot to delete them, they were intensionally commented out
| mode = "set" | ||
| } | ||
| mergedCov := coverage.Format(mode, res) | ||
| // if mode == "" { |
| default: | ||
| fmt.Fprintf(os.Stderr, "unrecognized cmd: %s\n", cmd) | ||
| os.Exit(1) | ||
| } | ||
| } | ||
|
|
||
| func mergeCover(files []string, outFile string, excludePrefix []string) error { | ||
| var mode string | ||
| // var mode string |
There was a problem hiding this comment.
Usually when not needed, line is removed not commented
| @@ -19,6 +19,10 @@ func Main(args []string) { | |||
| fmt.Print(strings.TrimPrefix(help, "\n")) | |||
| return | |||
| } | |||
| if cmd != "merge" && cmd != "compact" { | |||
| fmt.Fprintf(os.Stderr, "unrecognized cmd: %s\n", cmd) | |||
| return | |||
| @@ -19,6 +19,10 @@ func Main(args []string) { | |||
| fmt.Print(strings.TrimPrefix(help, "\n")) | |||
| return | |||
| } | |||
| if cmd != "merge" && cmd != "compact" { | |||
| fmt.Fprintf(os.Stderr, "unrecognized cmd: %s\n", cmd) | |||
| fmt.Fprintf(os.Stderr, "%v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| trace.Main(args) |
There was a problem hiding this comment.
May I suggest something?
Here and in previous code, you moved the os.Exit(1) inside the code, it's a bad pattern for me.
The code becomes almost impossible to test, because of the exit.
Maybe you made the change to be consistent with other part of code but I would have suggested to keep the error.
Why did you change ?
Also, this pattern leads to errors as os.Exit(1) can be forgotten
There was a problem hiding this comment.
I can help in refactoring it, and suggest you something if you agree.
You may accept it or not, but I would like to show what I have in mind.
One question, do you plan to support other exit codes than 1?
There was a problem hiding this comment.
@xhd2015 please let me know
No description provided.