Add get_issue_number function to webkitpy/common/checkout/scm/git.py

Add get_issue_number function to webkitpy/common/checkout/scm/git.py

This CL adds a "git cl issue" function that returns the issue number if one is associated with a branch and "None" otherwise.

Also adds unit test located in webkitpy/common/checkout/scm/scm_unittest.py

BUG=625254
Committed: https://crrev.com/56797ef5d98f8aaebe5c534ad760b4ad9befb194
Cr-Commit-Position: refs/heads/master@{#404065}

[dcampb, 2016-07-06 19:38:17]

Description was changed from

==========
changes


added function to rietveld


Merge branch 'master' of https://chromium.googlesource.com/chromium/src


safe browsing/testing added

BUG=
==========

to

==========
Added functionality to rietveld.py


Added a git cl issue function that returns the issue number if one is associated
with a branch and 'None' otherwise. 

I was not sure how to test this, due to having to run a command that would be
different on each machine. 


BUG=625254
==========

[dcampb, 2016-07-06 19:38:17]

dcampb@google.com changed reviewers:
+ dpranke@chromium.org, qyearsley@chromium.org

[qyearsley, 2016-07-06 21:08:43]

qyearsley@chromium.org changed reviewers:
+ raikiri@google.com

[dcampb, 2016-07-06 21:09:37]

dcampb@google.com changed reviewers:
- dpranke@chromium.org

[Dirk Pranke, 2016-07-06 21:22:37]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-07-06 21:22:38]

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:78: else:
I would enforce that the issue_number is legit as early in the public entry
points as possible, i.e., check them as the first thing in latest_try_jobs() and
get_latest_try_job_results(), rather than checking as part of a private
function.

But, given what I wrote below, it's probably better to just leave this file
unmodified.

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:94: def
get_cl_issue_number():
This method doesn't belong on this object, since you're talking to git, not to
Rietveld.

You should add a method to webkitpy/common/scm/git.py instead.

Also, if you use a pipe command and shell=True, this won't work on Windows. It's
easy enough to just get the output from `git cl issue` and parse it in python,
e.g. add a method to the Git class like:

  def issue_number(self):
    self._run_git(['cl', 'issue']).split()[2]

[qyearsley, 2016-07-06 21:38:56]

Additional comments about the CL description:

1. The verb in the CL title is usually not past tense, it's usually simple
present tense ("Add" rather than "Added").

2. The title should be specific enough that it clearly reminds you what
functionality was added, when you look back on committed CLs in the future. "Add
functionality" seems a little vague; "Add a function to get current Rietveld
issue number" is more specific.

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:94: def
get_cl_issue_number():
1. In other places in webkitpy, an Executive object (from
webkitpy.common.system.executive) is used; even if that module doesn't do
anything extra for us, there are two advantages to using it:
 - easily replaced with executive_mock.MockExecutive for tests
 - consistency with other code in this codebase

To use executive, you could make this function take an Executive object, and use
it when invoking a command. When testing this function, a MockExecutive would be
passed, and then you could assert that the command was called. Note, currently
there are two MockExecutive classes in
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
MockExecutive2 looks more useful because we can specify command output in the
test.

For example:

def get_cl_issue_number(executive):
    out = executive.run_command(['git', 'cl', 'issue'])
    ...

And then in the unit test:

    def test_get_cl_number(self):
        executive = MockExecutive2(output='Issue number: 12345
(http://crrev.com/12345)')
        issue_number = get_cl_issue_number(executive)
        self.assertEqual(issue_number, 12345)
        self.assertEqual(executive.calls[0], ['git', 'cl', 'issue'])

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:95:
issue_number = subprocess.check_output('git cl issue | cut -d" " -f3',
shell=True)
As I think you mentioned, this command using `cut` probably won't work on all
platforms.

So, for portability, we probably want process the output in Python itself.
Regular expressions would be one way of doing this:

  out = subprocess.check_output('git cl issue', shell=True)
  issue_number = re.search(r'\d+').group()
  return int(issue_number)

In the above, re.search(r'\d+') searches for the first substring the matches the
pattern \d+ (one or more digits), and the method group() returns the substring
that was found. 

Another way would be to use other string methods, e.g. `issue_number =
out.split()[2]`, but this is less flexible in case the output format changes.

[dcampb, 2016-07-06 21:42:06]

On 2016/07/06 at 21:22:38, dpranke wrote:
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):
> 
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:78: else:
> I would enforce that the issue_number is legit as early in the public entry
points as possible, i.e., check them as the first thing in latest_try_jobs() and
get_latest_try_job_results(), rather than checking as part of a private
function.
> 
> But, given what I wrote below, it's probably better to just leave this file
unmodified.
> 
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:94: def
get_cl_issue_number():
> This method doesn't belong on this object, since you're talking to git, not to
Rietveld.
> 
> You should add a method to webkitpy/common/scm/git.py instead.
> 
> Also, if you use a pipe command and shell=True, this won't work on Windows.
It's easy enough to just get the output from `git cl issue` and parse it in
python, e.g. add a method to the Git class like:
> 
>   def issue_number(self):
>     self._run_git(['cl', 'issue']).split()[2]

Does it matter if the result is a unicode string? or should I cast it as a str?

[dcampb, 2016-07-06 21:48:53]

Description was changed from

==========
Added functionality to rietveld.py


Added a git cl issue function that returns the issue number if one is associated
with a branch and 'None' otherwise. 

I was not sure how to test this, due to having to run a command that would be
different on each machine. 


BUG=625254
==========

to

==========
Add functionality to rietveld.py


Adds a git cl issue function that returns the issue number if one is associated
with a branch and 'None' otherwise. 

I was not sure how to test this, due to having to run a command that would be
different on each machine. 


BUG=625254
==========

[Dirk Pranke, 2016-07-06 21:53:01]

On 2016/07/06 21:42:06, dcampb wrote:
> On 2016/07/06 at 21:22:38, dpranke wrote:
> >
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> > File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py
(right):
> > 
> >
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> > third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:78: else:
> > I would enforce that the issue_number is legit as early in the public entry
> points as possible, i.e., check them as the first thing in latest_try_jobs()
and
> get_latest_try_job_results(), rather than checking as part of a private
> function.
> > 
> > But, given what I wrote below, it's probably better to just leave this file
> unmodified.
> > 
> >
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> > third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:94: def
> get_cl_issue_number():
> > This method doesn't belong on this object, since you're talking to git, not
to
> Rietveld.
> > 
> > You should add a method to webkitpy/common/scm/git.py instead.
> > 
> > Also, if you use a pipe command and shell=True, this won't work on Windows.
> It's easy enough to just get the output from `git cl issue` and parse it in
> python, e.g. add a method to the Git class like:
> > 
> >   def issue_number(self):
> >     self._run_git(['cl', 'issue']).split()[2]
> 
> Does it matter if the result is a unicode string? or should I cast it as a
str?

That's a good idea.

[qyearsley, 2016-07-06 21:59:23]

On 2016/07/06 21:42:06, dcampb wrote:
> On 2016/07/06 at 21:22:38, dpranke wrote:
> >
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> > File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py
(right):
> > 
> >
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> > third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:78: else:
> > I would enforce that the issue_number is legit as early in the public entry
> points as possible, i.e., check them as the first thing in latest_try_jobs()
and
> get_latest_try_job_results(), rather than checking as part of a private
> function.
> > 
> > But, given what I wrote below, it's probably better to just leave this file
> unmodified.
> > 
> >
>
https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
> > third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:94: def
> get_cl_issue_number():
> > This method doesn't belong on this object, since you're talking to git, not
to
> Rietveld.
> > 
> > You should add a method to webkitpy/common/scm/git.py instead.
> > 
> > Also, if you use a pipe command and shell=True, this won't work on Windows.
> It's easy enough to just get the output from `git cl issue` and parse it in
> python, e.g. add a method to the Git class like:
> > 
> >   def issue_number(self):
> >     self._run_git(['cl', 'issue']).split()[2]
> 
> Does it matter if the result is a unicode string? or should I cast it as a
str?

split() also works on unicode objects, and regular expression searches do too,
so it should work fine if it's a unicode; but since ultimately you want the
Rietveld issue number, it should also be OK to convert it to an int before; this
would ensure that it never returns some string that's not a sequence of digits.

Also, you can reply to specific comments in code review in-line, which makes it
easier to follow chains of comments about a particular part of the code.

[dcampb, 2016-07-06 22:13:07]

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:94: def
get_cl_issue_number():
On 2016/07/06 at 21:38:56, qyearsley wrote:
> 1. In other places in webkitpy, an Executive object (from
webkitpy.common.system.executive) is used; even if that module doesn't do
anything extra for us, there are two advantages to using it:
>  - easily replaced with executive_mock.MockExecutive for tests
>  - consistency with other code in this codebase
> 
> To use executive, you could make this function take an Executive object, and
use it when invoking a command. When testing this function, a MockExecutive
would be passed, and then you could assert that the command was called. Note,
currently there are two MockExecutive classes in
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
MockExecutive2 looks more useful because we can specify command output in the
test.
> 
> For example:
> 
> def get_cl_issue_number(executive):
>     out = executive.run_command(['git', 'cl', 'issue'])
>     ...
> 
> And then in the unit test:
> 
>     def test_get_cl_number(self):
>         executive = MockExecutive2(output='Issue number: 12345
(http://crrev.com/12345)')
>         issue_number = get_cl_issue_number(executive)
>         self.assertEqual(issue_number, 12345)
>         self.assertEqual(executive.calls[0], ['git', 'cl', 'issue'])

per dpranke@ comment, I moved the function to the git module. However, there is
no git_unittest.py. Should I add the unit test to the SCM unittest file and
import the git module?

[Dirk Pranke, 2016-07-06 22:33:29]

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py (right):

https://codereview.chromium.org/2129733002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py:94: def
get_cl_issue_number():
On 2016/07/06 22:13:06, dcampb wrote:
> per dpranke@ comment, I moved the function to the git module. However, there
is
> no git_unittest.py. Should I add the unit test to the SCM unittest file and
> import the git module?

Actually mocking all of that stuff up in scm_unittest.py for this to be properly
testable is more trouble than it's worth, so I wouldn't bother trying to write a
test for it.

[dcampb, 2016-07-06 23:30:59]

Description was changed from

==========
Add functionality to rietveld.py


Adds a git cl issue function that returns the issue number if one is associated
with a branch and 'None' otherwise. 

I was not sure how to test this, due to having to run a command that would be
different on each machine. 


BUG=625254
==========

to

==========
Adds get_issue_number function to git.py


Adds a git cl issue function that returns the issue number if one is associated
with a branch and 'None' otherwise. 

Also adds unit test located in scm_unittest.py


BUG=625254
==========

[dcampb, 2016-07-06 23:34:01]

Description was changed from

==========
Adds get_issue_number function to git.py


Adds a git cl issue function that returns the issue number if one is associated
with a branch and 'None' otherwise. 

Also adds unit test located in scm_unittest.py


BUG=625254
==========

to

==========
Adds get_issue_number function to webkitpy/common/checkout/scm/git.py


Adds a git cl issue function that returns the issue number if one is associated
with a branch and 'None' otherwise. 

Also adds unit test located in webkitpy/common/checkout/scm/scm_unittest.py


BUG=625254
==========

[Dirk Pranke, 2016-07-07 01:10:34]

lgtm. Bonus points for adding the test anyway ;).

[qyearsley, 2016-07-07 01:17:30]

Description was changed from

==========
Adds get_issue_number function to webkitpy/common/checkout/scm/git.py


Adds a git cl issue function that returns the issue number if one is associated
with a branch and 'None' otherwise. 

Also adds unit test located in webkitpy/common/checkout/scm/scm_unittest.py


BUG=625254
==========

to

==========
Add get_issue_number function to webkitpy/common/checkout/scm/git.py


This CL adds a "git cl issue" function that returns the issue number if one is
associated with a branch and "None" otherwise. 

Also adds unit test located in webkitpy/common/checkout/scm/scm_unittest.py


BUG=625254
==========

[qyearsley, 2016-07-07 01:18:26]

On 2016/07/07 at 01:10:34, dpranke wrote:
> lgtm. Bonus points for adding the test anyway ;).

LGTM too

[dcampb, 2016-07-07 02:28:04]

The CQ bit was checked by dcampb@google.com

[commit-bot: I haz the power, 2016-07-07 02:28:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 03:45:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-07 03:47:44]

Description was changed from

==========
Add get_issue_number function to webkitpy/common/checkout/scm/git.py


This CL adds a "git cl issue" function that returns the issue number if one is
associated with a branch and "None" otherwise. 

Also adds unit test located in webkitpy/common/checkout/scm/scm_unittest.py


BUG=625254
==========

to

==========
Add get_issue_number function to webkitpy/common/checkout/scm/git.py

This CL adds a "git cl issue" function that returns the issue number if one is
associated with a branch and "None" otherwise.

Also adds unit test located in webkitpy/common/checkout/scm/scm_unittest.py

BUG=625254

Committed: https://crrev.com/56797ef5d98f8aaebe5c534ad760b4ad9befb194
Cr-Commit-Position: refs/heads/master@{#404065}
==========

[commit-bot: I haz the power, 2016-07-07 03:47:45]

Patchset 4 (id:??) landed as
https://crrev.com/56797ef5d98f8aaebe5c534ad760b4ad9befb194
Cr-Commit-Position: refs/heads/master@{#404065}