Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(35)

Issue 8842043: cmd/juju: simplify status

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by rog
Modified:
9 years, 7 months ago
Reviewers:
mp+159503, fwereade, thumper, TheMue
Visibility:
Public.

Description

cmd/juju: simplify status After all Frank's hard work trying to work with the existing status structure (thanks!), I still found it very hard to understand what was going on, and while trying to do so, a cunning plan came to mind which I couldn't resist. This is the result. The tests are unchanged from lp:~themue/juju-core/020-cmd-status-subordinates https://code.launchpad.net/~rogpeppe/juju-core/292-simpler-status/+merge/159503 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 13

Patch Set 2 : cmd/juju: simplify status #

Patch Set 3 : cmd/juju: simplify status #

Patch Set 4 : cmd/juju: simplify status #

Total comments: 4

Patch Set 5 : cmd/juju: simplify status #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+430 lines, -225 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/status.go View 1 2 3 5 chunks +276 lines, -209 lines 2 comments Download
M cmd/juju/status_test.go View 1 2 3 6 chunks +146 lines, -16 lines 1 comment Download
M state/service.go View 1 1 chunk +6 lines, -0 lines 1 comment Download

Messages

Total messages: 8
rog
Please take a look.
9 years, 7 months ago (2013-04-17 21:43:09 UTC) #1
fwereade
LGTM, this is very neat. https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode78 cmd/juju/status.go:78: insts, err := env.AllInstances() ...
9 years, 7 months ago (2013-04-17 22:04:07 UTC) #2
rog
Please take a look. https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/8842043/diff/1/cmd/juju/status.go#newcode78 cmd/juju/status.go:78: insts, err := env.AllInstances() On ...
9 years, 7 months ago (2013-04-17 22:31:45 UTC) #3
thumper
LGTM
9 years, 7 months ago (2013-04-17 22:43:47 UTC) #4
fwereade
still LGTM with trivials https://codereview.appspot.com/8842043/diff/10001/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/8842043/diff/10001/cmd/juju/status.go#newcode140 cmd/juju/status.go:140: return I wanted to say ...
9 years, 7 months ago (2013-04-17 22:49:26 UTC) #5
rog
*** Submitted: cmd/juju: simplify status After all Frank's hard work trying to work with the ...
9 years, 7 months ago (2013-04-17 22:50:25 UTC) #6
TheMue
Nice refactoring, would have been the next step. https://codereview.appspot.com/8842043/diff/14001/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/8842043/diff/14001/cmd/juju/status.go#newcode55 cmd/juju/status.go:55: var ...
9 years, 7 months ago (2013-04-18 06:50:50 UTC) #7
rog
9 years, 7 months ago (2013-04-18 07:28:19 UTC) #8
https://codereview.appspot.com/8842043/diff/14001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8842043/diff/14001/cmd/juju/status.go#newcode55
cmd/juju/status.go:55: var ctxt statusContext
On 2013/04/18 06:50:50, TheMue wrote:
> What does the postfix "t" stand for? Would prefer "sctx" for a status context.

sorry, i've always used "ctxt" as short form for "context". just habit.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b