Refactor the way that SCM changes are tracked and aggregated in rebaseline.py.

Refactor the way that SCM changes are tracked and aggregated in rebaseline.py.

This is a preliminary refactoring CL which is intended to consolidate related logic into one place, and prepare for the real change that I want to make: That is, apply git changes only once in _rebaseline after all changes are made, to fix http://crbug.com/639410.

I want to make two follow-up changes after this:
 (1) Change _rebaseline to apply git changes only once after all commands are run.
 (2) Refactor and simplify ChangeSet, _serial_commands, and _run_in_parallel.

BUG=639410
Committed: https://crrev.com/9dc06fa0fd9e3ffb68b4feeef5158ab54f2ee6b6
Cr-Commit-Position: refs/heads/master@{#419038}

[qyearsley, 2016-09-15 18:20:44]

Description was changed from

==========
Refactor the way that SCM changes are tracked and aggregated.

BUG=639410
==========

to

==========
Refactor the way that SCM changes are tracked and aggregated in rebaseline.py

BUG=639410
==========

[qyearsley, 2016-09-15 18:20:44]

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

[qyearsley, 2016-09-15 18:47:39]

Description was changed from

==========
Refactor the way that SCM changes are tracked and aggregated in rebaseline.py

BUG=639410
==========

to

==========
Refactor the way that SCM changes are tracked and aggregated in rebaseline.py

This is a preliminary refactoring CL which is intended to consolidate related
logic into one place, and prepare for the real change that I want to make: That
is, apply git changes only once in _rebaseline after all changes are made, to
fix http://crbug.com/639410.

I want to make two follow-up changes after this:
 (1) Change _rebaseline to apply git changes only once after all commands are
run.
 (2) Refactor and simplify ChangeSet, _serial_commands, and _run_in_parallel.

BUG=639410
==========

[qyearsley, 2016-09-15 18:47:39]

qyearsley@chromium.org changed reviewers:
+ jeffcarp@chromium.org

[qyearsley, 2016-09-15 18:48:25]

Description was changed from

==========
Refactor the way that SCM changes are tracked and aggregated in rebaseline.py

This is a preliminary refactoring CL which is intended to consolidate related
logic into one place, and prepare for the real change that I want to make: That
is, apply git changes only once in _rebaseline after all changes are made, to
fix http://crbug.com/639410.

I want to make two follow-up changes after this:
 (1) Change _rebaseline to apply git changes only once after all commands are
run.
 (2) Refactor and simplify ChangeSet, _serial_commands, and _run_in_parallel.

BUG=639410
==========

to

==========
Refactor the way that SCM changes are tracked and aggregated in rebaseline.py.

This is a preliminary refactoring CL which is intended to consolidate related
logic into one place, and prepare for the real change that I want to make: That
is, apply git changes only once in _rebaseline after all changes are made, to
fix http://crbug.com/639410.

I want to make two follow-up changes after this:
 (1) Change _rebaseline to apply git changes only once after all commands are
run.
 (2) Refactor and simplify ChangeSet, _serial_commands, and _run_in_parallel.

BUG=639410
==========

[qyearsley, 2016-09-15 20:02:10]

qyearsley@chromium.org changed reviewers:
+ wkorman@chromium.org
- jeffcarp@chromium.org

[Dirk Pranke, 2016-09-15 21:03:02]

lgtm

https://codereview.chromium.org/2347643002/diff/60001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(right):

https://codereview.chromium.org/2347643002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:415: A
ChangeSet object.
I'm not generally a fan of the comments that document the args and return values
so verbosely, but if you continue to find them useful, you might consider
switching to 2.7-compatible type hints a la PEP-484, e.g.:

def _serial_commands(command_results):
    # type: List[Tuple[int, str, str]] -> ChangeSet
    """...""" 

which is perhaps slightly less informative but way more compact and it follows a
particular syntax.

(See https://www.python.org/dev/peps/pep-0484/#id46 )

[wkorman, 2016-09-15 21:05:06]

lgtm

https://codereview.chromium.org/2347643002/diff/60001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(right):

https://codereview.chromium.org/2347643002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:105:
self.lines_to_remove[test].append(builder)
Maybe the builder list should be a set too, though not sure it's possible w/
current logic to add one builder multiple times.

[qyearsley, 2016-09-15 22:20:23]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-09-15 22:20:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2347643002/#ps80001
(title: "Removed Args/Returns from _serial_commands docstring.")

[qyearsley, 2016-09-15 22:20:30]

https://codereview.chromium.org/2347643002/diff/60001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
(right):

https://codereview.chromium.org/2347643002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:105:
self.lines_to_remove[test].append(builder)
On 2016/09/15 at 21:05:06, wkorman wrote:
> Maybe the builder list should be a set too, though not sure it's possible w/
current logic to add one builder multiple times.

That makes sense; I should do this in the follow-up, at the same time as making
it so that files to add/delete are sets.

https://codereview.chromium.org/2347643002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:415: A
ChangeSet object.
On 2016/09/15 at 21:03:02, Dirk Pranke wrote:
> I'm not generally a fan of the comments that document the args and return
values so verbosely, but if you continue to find them useful, you might consider
switching to 2.7-compatible type hints a la PEP-484, e.g.:
> 
> def _serial_commands(command_results):
>     # type: List[Tuple[int, str, str]] -> ChangeSet
>     """...""" 
> 
> which is perhaps slightly less informative but way more compact and it follows
a particular syntax.
> 
> (See https://www.python.org/dev/peps/pep-0484/#id46 )

I'm slightly hesitant to start adopting that format here, since it would then be
the only place in the codebase where that is done.

I see that this is a bit verbose; ideally, as usual, the return type and arg
types should be able to be read from the code itself. In this case since there's
one return at the end of the function, I think it's reasonable to remove the
Args/Returns section from the docstring now?

[commit-bot: I haz the power, 2016-09-15 22:20:55]

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

[qyearsley, 2016-09-15 22:22:53]

On 2016/09/15 at 22:20:30, qyearsley wrote:
> I see that this is a bit verbose; ideally, as usual, the return type and arg
types should be able to be read from the code itself. In this case since there's
one return at the end of the function, I think it's reasonable to remove the
Args/Returns section from the docstring now?

To clarify, what I meant was that: ideally, in general, the code should make
return and arg types clear.

(I think in this case it should be clear enough, so I think it's OK to remove
part of the docstring.)

[commit-bot: I haz the power, 2016-09-15 23:37:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-15 23:39:30]

Description was changed from

==========
Refactor the way that SCM changes are tracked and aggregated in rebaseline.py.

This is a preliminary refactoring CL which is intended to consolidate related
logic into one place, and prepare for the real change that I want to make: That
is, apply git changes only once in _rebaseline after all changes are made, to
fix http://crbug.com/639410.

I want to make two follow-up changes after this:
 (1) Change _rebaseline to apply git changes only once after all commands are
run.
 (2) Refactor and simplify ChangeSet, _serial_commands, and _run_in_parallel.

BUG=639410
==========

to

==========
Refactor the way that SCM changes are tracked and aggregated in rebaseline.py.

This is a preliminary refactoring CL which is intended to consolidate related
logic into one place, and prepare for the real change that I want to make: That
is, apply git changes only once in _rebaseline after all changes are made, to
fix http://crbug.com/639410.

I want to make two follow-up changes after this:
 (1) Change _rebaseline to apply git changes only once after all commands are
run.
 (2) Refactor and simplify ChangeSet, _serial_commands, and _run_in_parallel.

BUG=639410

Committed: https://crrev.com/9dc06fa0fd9e3ffb68b4feeef5158ab54f2ee6b6
Cr-Commit-Position: refs/heads/master@{#419038}
==========

[commit-bot: I haz the power, 2016-09-15 23:39:31]

Patchset 5 (id:??) landed as
https://crrev.com/9dc06fa0fd9e3ffb68b4feeef5158ab54f2ee6b6
Cr-Commit-Position: refs/heads/master@{#419038}