-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
A few nits.
cmd/dep/status.go
Outdated
@@ -65,6 +67,89 @@ type statusCommand struct { | |||
modified bool | |||
} | |||
|
|||
type Outputter interface { |
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 know this is in package main
, but the interface should be private.
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
cmd/dep/status.go
Outdated
@@ -78,10 +163,20 @@ func (cmd *statusCommand) Run(ctx *dep.Ctx, args []string) error { | |||
sm.UseDefaultSignalHandling() | |||
defer sm.Release() | |||
|
|||
var out Outputter | |||
if cmd.detailed { |
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.
use a switch
to set the Outputter
instead of all the ifs and elses.
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
cmd/dep/status.go
Outdated
} | ||
|
||
func (out *jsonOutput) MissingFooter() { | ||
json.NewEncoder(os.Stdout).Encode(out.missing) |
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.
should be out.w
instead of os.Stdout
?
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.
right
|
||
type jsonOutput struct { | ||
w io.Writer | ||
basic []*BasicStatus |
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.
Do the two status slices need to be pointers?
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.
Yes, avoiding copies of marshal
@groob Thanks for your review. |
Sorry for the delay on this - merging! |
[WIP] Refactor sourceMgr
implement for
-json