Merge lp:~fwereade/pyjuju/go-cmd-context into lp:pyjuju/go
Status: | Rejected |
---|---|
Rejected by: | William Reade |
Proposed branch: | lp:~fwereade/pyjuju/go-cmd-context |
Merge into: | lp:pyjuju/go |
Prerequisite: | lp:~fwereade/pyjuju/go-log-type |
Diff against target: |
722 lines (+336/-123) 13 files modified
cmd/command.go (+15/-49) cmd/context.go (+117/-0) cmd/context_test.go (+148/-0) cmd/juju/bootstrap.go (+3/-3) cmd/juju/main.go (+1/-1) cmd/jujud/agent.go (+5/-5) cmd/jujud/initzk.go (+6/-6) cmd/jujud/machine.go (+4/-3) cmd/jujud/main.go (+1/-1) cmd/jujud/provisioning.go (+5/-2) cmd/jujud/unit.go (+5/-4) cmd/supercommand.go (+13/-36) cmd/supercommand_test.go (+13/-13) |
To merge this branch: | bzr merge lp:~fwereade/pyjuju/go-cmd-context |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+95602@code.launchpad.net |
Description of the change
add cmd.Context to allow for commands with non-global output/logging
prereq: lp:~fwereade/juju/go-log-type
The top-level Main, Run, Parse and NewFlagSet function have all become methods
on the new Context type, as has SuperCommand.
are both used by code outside this package, have been retained for
convenience's sake and act as they did before.
The Command interface has changed slightly; Run now expects a *Context, and
ParsePositional can now return a []string to indicate that the Command should
be Parsed again with the returned args; this allows us to move the re-Parsing
logic to Context.Parse; even though SuperCommand is the only user, this
allows us to avoid passing a Context into ParsePositional, which feels quite
inappropriate. (Given that every Command has to implement ParsePositional,
it's better to add additional nil return values everywhere than to pass an
unused Context around everywhere).
This doesn't feel like an overwhelmingly large change, but it doesn't diff
very nicely. Most of the changes should be explained by the above; there's
also an unrelated type rename in supercommand_
like a good idea at the time (and still doesn't feel like a bad one, even if
it adds to the noise a bit).
Unmerged revisions
- 83. By William Reade
-
merge parent
- 82. By William Reade
-
added direct tests for Context
- 81. By William Reade
-
rearranged cmd output to avoid reprinting errors/usage
- 80. By William Reade
-
er, remove the Exit field itself...
- 79. By William Reade
-
Exit doesn't really want to be part of Context, I think...
- 78. By William Reade
-
added cmd.Context, which now exposes most of the original top-level functions, and SuperCommand's erstwhile initOutput method; retained top-level Parse and Main functions, which act as before, but by acting via a default Context rather than by directly hitting os.Exit, etc
- 77. By William Reade
-
add log.Log type, with an instance always available as log.Global
Reviewers: mp+95602_ code.launchpad. net,
Message:
Please take a look.
Description:
prereq: lp:~fwereade/juju/go-log-type
The top-level Main, Run, Parse and NewFlagSet function have all become initOutput. Main and Parse,
methods
on the new Context type, as has SuperCommand.
which
are both used by code outside this package, have been retained for
convenience's sake and act as they did before.
The Command interface has changed slightly; Run now expects a *Context,
and
ParsePositional can now return a []string to indicate that the Command
should
be Parsed again with the returned args; this allows us to move the
re-Parsing
logic to Context.Parse; even though SuperCommand is the only user, this
allows us to avoid passing a Context into ParsePositional, which feels
quite
inappropriate. (Given that every Command has to implement
ParsePositional,
it's better to add additional nil return values everywhere than to pass
an
unused Context around everywhere).
This doesn't feel like an overwhelmingly large change, but it doesn't test.go because, er, it
diff
very nicely. Most of the changes should be explained by the above;
there's
also an unrelated type rename in supercommand_
seemed
like a good idea at the time (and still doesn't feel like a bad one,
even if
it adds to the noise a bit).
https:/ /code.launchpad .net/~fwereade/ juju/go- cmd-context/ +merge/ 95602
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/5719050/
Affected files: bootstrap. go machine. go provisioning. go d_test. go
M cmd/command.go
A cmd/context.go
A cmd/context_test.go
M cmd/juju/
M cmd/juju/main.go
M cmd/jujud/agent.go
M cmd/jujud/initzk.go
M cmd/jujud/
M cmd/jujud/main.go
M cmd/jujud/
M cmd/jujud/unit.go
M cmd/supercommand.go
M cmd/supercomman
M log/log.go
M log/log_test.go
M store/store_test.go