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

Issue 2949: Refactor upload.py in preparation for other VCS implementations. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years, 7 months ago by Andi
Modified:
11 years, 8 months ago
Base URL:
http://rietveld.googlecode.com/svn/trunk/
Visibility:
Public.

Description

We have currently three patches for other version control systems (git, hg and bzr) and all of them have these refactorings. * Add missing GuessBase() to abstract VersionControlSystem * Add GuessVCS() helper that tries to guess the used version control system. * Some minor cleanup.

Patch Set 1 #

Patch Set 2 : Docstring corrected. #

Patch Set 3 : Make GuessBase a Subversion only feature. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -9 lines) Patch
static/upload.py View 1 2 9 chunks +31 lines, -9 lines 4 comments Download

Messages

Total messages: 9
Andi Albrecht
15 years, 7 months ago (2008-08-17 07:22:30 UTC) #1
Evan Martin
This looks good to me. Minor comments below. http://codereview.appspot.com/2949/diff/23/3 File static/upload.py (right): http://codereview.appspot.com/2949/diff/23/3#newcode503 Line 503: ...
15 years, 7 months ago (2008-08-17 08:44:25 UTC) #2
Andi Albrecht
http://codereview.appspot.com/2949/diff/23/3 File static/upload.py (right): http://codereview.appspot.com/2949/diff/23/3#newcode503 Line 503: """Returns the base URL of the repository. On ...
15 years, 7 months ago (2008-08-17 10:58:03 UTC) #3
Andi Albrecht
GuessBase() is now removed from abstract VersionControlSystem again and RealMain() modified to call GuessBase() only ...
15 years, 7 months ago (2008-08-17 19:03:12 UTC) #4
Evan Martin
LGTM http://codereview.appspot.com/2949/diff/25/26 File static/upload.py (right): http://codereview.appspot.com/2949/diff/25/26#newcode816 Line 816: ErrorExit("Use '--local_base' to upload base files.") Maybe ...
15 years, 7 months ago (2008-08-17 23:25:45 UTC) #5
Evan Martin
http://codereview.appspot.com/2949/diff/25/26 File static/upload.py (right): http://codereview.appspot.com/2949/diff/25/26#newcode788 Line 788: if os.path.isdir(os.path.join(os.getcwd(), '.svn')): shorter: "if os.path.isdir('.svn'):"
15 years, 7 months ago (2008-08-18 03:31:06 UTC) #6
GvR
Do you want me to review this too?
15 years, 7 months ago (2008-08-18 15:05:18 UTC) #7
GvR
LGTM. Agreed with Evan's comments though.
15 years, 7 months ago (2008-08-18 20:50:01 UTC) #8
Andi Albrecht
15 years, 7 months ago (2008-08-18 21:04:00 UTC) #9
Committed r252 with two minor changes (see comments below).

http://codereview.appspot.com/2949/diff/25/26
File static/upload.py (right):

http://codereview.appspot.com/2949/diff/25/26#newcode788
Line 788: if os.path.isdir(os.path.join(os.getcwd(), '.svn')):
On 2008/08/18 03:31:06, Evan Martin wrote:
> shorter: "if os.path.isdir('.svn'):"

Done.

http://codereview.appspot.com/2949/diff/25/26#newcode816
Line 816: ErrorExit("Use '--local_base' to upload base files.")
On 2008/08/17 23:25:45, Evan Martin wrote:
> Maybe we should just turn on options.local_base when base isn't available?  I
> guess that's maybe beyond the scope of this review.

I've added a TODO here. The current upload.py shouldn't reach this point.
Sign in to reply to this message.

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