solve #6 #8
solve #6 #8
Conversation
fabulousduck
commented
Sep 20, 2018
•
fabulousduck
commented
- solves index out of range error when no subcommands are provided #6 by providing a list of possible arguments when no argument is provided.
- applies style changes according to gofmt.
- introduces one new TODO (Use a map to store snitch argument options #9), this TODO is a nice to have.
main.go
Outdated
| panic(fmt.Sprintf("`%s` unknown command", os.Args[1])) | ||
| } | ||
| } else { | ||
| //TODO (#7) implement a map for options instead of println'ing them all there |
There was a problem hiding this comment.
Please, check out the format of reported TODOs It should look like
// TODO(#7): implement a map for options instead of println'ing them all there
Also, you please leave unreported TODOs I'll report all of them myself once the PR is approved. This is because some TODOs may be eliminated or changed during the review. This one is fine, though. Let's keep it. 👍
main.go
Outdated
| } else { | ||
| //TODO (#7) implement a map for options instead of println'ing them all there | ||
| //also, not sure these descriptions are exactly what rexim means by them | ||
| fmt.Printf("snitch [opt]\n\tlist: lists all possible subcommands\n\treport: reports an issue to github\n") |
There was a problem hiding this comment.
Doesn't Go have multiline strings?
main.go
Outdated
| @@ -124,7 +124,7 @@ func main() { | |||
| panic(fmt.Sprintf("`%s` unknown command", os.Args[1])) | |||
| } | |||
| } else { | |||
| //TODO (#7) implement a map for options instead of println'ing them all there | |||
| //TODO (#8) implement a map for options instead of println'ing them all there | |||
There was a problem hiding this comment.
You were correct before the change. It should be #7. :)
There was a problem hiding this comment.
oh, i thought #7 was the CI issue lmao
There was a problem hiding this comment.
this does bring a new a new interesting issue, that TODO's do need to be kept in sync with whats reported on github
There was a problem hiding this comment.
@fabulousduck lol my bad. The TODO should have the id of the issue it's associated with. So it should be TODO(#9). We both fucked up here. :D
There was a problem hiding this comment.
@fabulousduck that's why it is better to just leave unreported TODO and let me fuck up alone :D
|
@fabulousduck looks good to me. Multiline strings are not necessary for now. Thank you for the PR! |
