Issue 7004 - Actually return git's result

Issue 7004 - Actually return git's result

A regression in the first always returned "1" instead of the actual result.

[tlucas, 2018-10-08 13:24:36]

Patch Set 1;
 * Don't count lines of the result, return it instead

[Sebastian Noack, 2018-10-08 13:28:25]

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?

[tlucas, 2018-10-08 13:35:28]

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', '', result)
On 2018/10/08 13:28:25, Sebastian Noack wrote:
> Wouldn't strip() do?

It does :) Done.

[kzar, 2018-10-08 13:54:06]

LGTM

[Sebastian Noack, 2018-10-08 14:01:04]

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 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.

[tlucas, 2018-10-08 14:19:42]

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:
> 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?

[Sebastian Noack, 2018-10-08 14:26:06]

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.