Project Sync transitioned to new success/error handling#98
Conversation
grayside
left a comment
grayside
left a comment
There was a problem hiding this comment.
Overall this looks good. It's the kind of PR that any holes will be missing from the diff. This also needs a user experience review for the messages in the context of any other command logging. On that basis, I think I'd like to take it out for a spin before finalizing an approval, unless you think we should get this in and be prepared for another follow-up for gaps and string adjustments.
| } | ||
| } else { | ||
| cmd.out.Error.Fatal(err) | ||
| var workingDir, err = cmd.DeriveLocalSyncPath(cmd.Config, ctx.String("dir")) |
There was a problem hiding this comment.
Why does this work without the :=? When I've tried things like this before, I'd have err yelling at me.
There was a problem hiding this comment.
:= is just a shorthand for not needing to var it. The var and = I believe is redefining it.
| cmd.out.Error.Fatal(err) | ||
| var workingDir, err = cmd.DeriveLocalSyncPath(cmd.Config, ctx.String("dir")) | ||
| if err != nil { | ||
| return cmd.Error(err.Error(), "SYNC-PATH-ERROR", 12) |
There was a problem hiding this comment.
Hm, maybe we should define cmd.EXTERNAL_ERROR_CODE and so on for more clarity about the numbers? I realize that's outside immediate scope but reading this out of the context of the other PR this immediately jumps out at me as "why is this a number 12?"
There was a problem hiding this comment.
I think some constants for standard error codes are probably a good idea. We can tackle that in another PR
2 participants
This was the big daddy, but I think I got it. Required a lil bit of refactoring to make it all work.