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

Issue 29904555: Issue 7004 - Actually return git's result (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 2 weeks ago by tlucas
Modified:
9 months, 2 weeks ago
Visibility:
Public.

Description

Issue 7004 - Actually return git's result A regression in the first always returned "1" instead of the actual result.

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -2 lines) Patch
M packager.py View 1 1 chunk +5 lines, -2 lines 3 comments Download

Messages

Total messages: 7
tlucas
Patch Set 1; * Don't count lines of the result, return it instead
9 months, 2 weeks ago (2018-10-08 13:24:36 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29904555/diff/29904556/packager.py File packager.py (right): https://codereview.adblockplus.org/29904555/diff/29904556/packager.py#newcode79 packager.py:79: return re.sub(r'\D', '', result) Wouldn't strip() do?
9 months, 2 weeks ago (2018-10-08 13:28:25 UTC) #2
tlucas
Patch Set 2: * use str.strip() https://codereview.adblockplus.org/29904555/diff/29904556/packager.py File packager.py (right): https://codereview.adblockplus.org/29904555/diff/29904556/packager.py#newcode79 packager.py:79: return re.sub(r'\D', '', ...
9 months, 2 weeks ago (2018-10-08 13:35:28 UTC) #3
kzar
LGTM
9 months, 2 weeks ago (2018-10-08 13:54:06 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29904555/diff/29904558/packager.py File packager.py (right): https://codereview.adblockplus.org/29904555/diff/29904558/packager.py#newcode79 packager.py:79: return result.strip() Perhaps we can unify the code paths ...
9 months, 2 weeks ago (2018-10-08 14:01:04 UTC) #5
tlucas
https://codereview.adblockplus.org/29904555/diff/29904558/packager.py File packager.py (right): https://codereview.adblockplus.org/29904555/diff/29904558/packager.py#newcode79 packager.py:79: return result.strip() On 2018/10/08 14:01:04, Sebastian Noack wrote: > ...
9 months, 2 weeks ago (2018-10-08 14:19:42 UTC) #6
Sebastian Noack
9 months, 2 weeks ago (2018-10-08 14:26:06 UTC) #7
https://codereview.adblockplus.org/29904555/diff/29904558/packager.py
File packager.py (right):

https://codereview.adblockplus.org/29904555/diff/29904558/packager.py#newcode79
packager.py:79: return result.strip()
On 2018/10/08 14:19:42, tlucas wrote:
> On 2018/10/08 14:01:04, Sebastian Noack wrote:
> > Perhaps we can unify the code paths here:
> > 
> >   if Mercurial().istype(baseDir):
> >       output = subprocess.check_output(['hg', 'id', '-R', baseDir, '-n'])
> >   else:
> >       output = subprocess.check_output(
> >           ['git', 'rev-list', '--count', '--branches', '--tags'],
> >           cwd=baseDir,
> >       )
> >   return re.sub(r'\D', '', output)
> > 
> > 1. While strip() would be sufficient for Git it's not for Mercurial (as it
> might
> > add a plus character at the end). So if the code path is unified we have to
go
> > back using the regexp for both cases.
> > 2. Explicitly checking for Git is redundant since if it's neither a
Mercurial
> > nor a Git repository CalledProcess Error is raised which is handled below.
> 
> I see how this would improve the code, but since we already agreed on
eventually
> moving the buildtools to Node.js, i'd rather not introduce any cosmetic
changes.
> 
> What do you think?

I think the requested change is trivial, and makes this code easier to maintain
for the the time being (and who knows how long that will be in the end). If we'd
be talking about a larger refactoring, I'd agree of course.

But it's probably not worth to argue about it. LGTM, either with or without my
suggested change.
Sign in to reply to this message.

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