This patch set pull all the subversion-specific bits of upload.py into a single class. This ...
15 years, 9 months ago
(2008-07-18 06:09:05 UTC)
#1
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.
On 2008/07/18 06:09:05, Evan Martin wrote: > This patch set pull all the subversion-specific bits ...
15 years, 9 months ago
(2008-07-18 06:09:31 UTC)
#2
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.
I don't have time to review this in detail, but I'm all for this in ...
15 years, 9 months ago
(2008-07-18 14:18:08 UTC)
#3
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.
I have some comments for some more refactoring. Also, this patch doesn't apply cleanly any ...
15 years, 9 months ago
(2008-07-30 18:13:25 UTC)
#5
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.
On 2008/07/30 18:13:25, GvR wrote: > I have some comments for some more refactoring. > ...
15 years, 9 months ago
(2008-07-30 23:22:58 UTC)
#6
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.
On 2008/07/31 17:29:37, GvR wrote: > Checked in as r182, with some tweaks: > > ...
15 years, 9 months ago
(2008-07-31 22:07:32 UTC)
#8
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...
Issue 2589: abstract out subversion calls
(Closed)
Created 15 years, 9 months ago by Evan Martin
Modified 14 years, 9 months ago
Reviewers: Andi Albrecht, GvR, ondrej.certik
Base URL: http://rietveld.googlecode.com/svn/trunk/
Comments: 4