abstract out subversion calls

[Evan Martin, 2008-07-18 06:09:05]

This patch set pull all the subversion-specific bits of upload.py into a single
class.  This lets me (in a followup patch) use git with upload.py as well.

[Evan Martin, 2008-07-18 06:09:31]

On 2008/07/18 06:09:05, Evan Martin wrote:
> This patch set pull all the subversion-specific bits of upload.py into a
single
> class.  This lets me (in a followup patch) use git with upload.py as well.

BTW, I made a point of not making any functional change so this is easier to
review.

[GvR, 2008-07-18 14:18:08]

I don't have time to review this in detail, but I'm all for this in principle. 
You should probably wait until the next set of changes to upload.py is submitted
though.

[ondrej.certik, 2008-07-19 19:58:05]

I think this is a good patch, it makes the upload.py more modular. +1 from me.

[GvR, 2008-07-30 18:13:25]

I have some comments for some more refactoring.

Also, this patch doesn't apply cleanly any more since some recent changes to
upload.py.  Please regenerate the patch before making other changes.

http://codereview.appspot.com/2589/diff/41/61
File static/upload.py (right):

http://codereview.appspot.com/2589/diff/41/61#newcode475
Line 475: class SubversionVCS:
I think you should start with an abstract base class that defines some methods
(see below) and then derive this class from that.  The ABC should act as
documentation for what you have to implement.

http://codereview.appspot.com/2589/diff/41/61#newcode531
Line 531: """Get a list of files unknown to Subversion."""
Subversion -> the VCS.

http://codereview.appspot.com/2589/diff/41/61#newcode675
Line 675: def UploadBaseFiles(issue, rpc_server, vcs, patch_list, patchset,
options):
I wonder if this shouldn't be a method of the vcs (base) class?  Then instead of
passing vcs you'd be using self.GetBaseFile(...).  In general I think an even
more OO approach might be beneficial.

http://codereview.appspot.com/2589/diff/41/61#newcode709
Line 709: def CheckForUnknownFiles(vcs):
Looks like another candidate for being a method on the vcs base class.

[Evan Martin, 2008-07-30 23:22:58]

On 2008/07/30 18:13:25, GvR wrote:
> I have some comments for some more refactoring.
> 
> Also, this patch doesn't apply cleanly any more since some recent changes to
> upload.py.  Please regenerate the patch before making other changes.

Ok, I've manually reimplemented this against the latest version just to be sure
I didn't miss any new code.

I think I've also addressed all your review comments.

Please look again.

[GvR, 2008-07-31 17:29:37]

Checked in as r182, with some tweaks:

- made it a new-style class
- raise NotImplementedError instead of RuntimeError

[GvR, 2008-07-31 22:07:32]

On 2008/07/31 17:29:37, GvR wrote:
> Checked in as r182, with some tweaks:
> 
> - made it a new-style class
> - raise NotImplementedError instead of RuntimeError

Evan, please close the issue...