launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Thank you for this branch.  Cleaning out the no-longer-working doctests is nice too, though hooking them up again, insofar as they actually were documentation, might not have been horrible.  Thank you also for changing "directory" to "repository".

I don't want to forget the branch argument: if we have time, we should add it.  Please add a low priority card to the kanban board for this task.

Using get to update an existing checkout is interesting, and I'm not sure it is the right thing.  That's the intended domain of update, which also is supposed to shelve changes before the pull and unshelve them after the pull.  I suggest that we rip that behavior out from get.  (Note that IMO we can also rip the branch command too, if that makes anything easier; but that can wait till another branch too.  Note further that we can rip out the install command, but that's trickier because lxcinstall defers to it ATM, so I definitely think that ripping install out needs to be its own branch.)

I was curious about the "Repository {0} is corrupted" stuff when I saw it too...did you check with frankban before you removed it?  If not, please do.  I think either removing it or commenting on why we think it is necessary are both good possible directions.

If there is no sandbox_dir, and only a checkout_dir, can we infer that the checkout is actually a branch (i.e., lightweight=False)?

ISTR someone was going to check on why we have --2a as an argument.  That should be the default for all versions of bzr that we encounter.  If it is not, I think a comment is appropriate.  Otherwise, let's remove it.

I skimmed the rest, and it looked good.  I'm not going to approve pending changes because I think there may need to be a discussion about one or the both of inferring the "lightweight" value and ripping out get's "update" behavior.  If someone else wants to take over the rest of the review, that would be cool.

Thanks again,

Gary
-- 
https://code.launchpad.net/~benji/lpsetup/bug-1016645-add-command-line-options/+merge/112588
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/lpsetup/bug-1016645-add-command-line-options into lp:lpsetup.