[pinpoint] Use _JobState.Differences() in _JobState.Explore().

[pinpoint] Use _JobState.Differences() in _JobState.Explore().

Review-Url: https://chromiumcodereview.appspot.com/3013153002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/1a128a7a63b4596a73c063d053572a0120ed6e73

[dtu, 2017-09-18 20:54:37]

dtu@chromium.org changed reviewers:
+ perezju@chromium.org, simonhatch@chromium.org, sullivan@chromium.org

[frankangel340, 2017-09-19 08:19:13]

frankangel340@gmail.com changed reviewers:
+ frankangel340@gmail.com

[perezju, 2017-09-19 13:54:53]

lgtm w/suggestion

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/job.py (right):

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:248: # This loop adds Changes to the
_changes list while looping through it.
Actually, it doesn't. Because of "tuple" you're now iterating through a copy of
the original list. Which is fine!

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:288: yield index, change_b
Looking at what clients actually want, maybe it would be neater if you yield
(change_a, change_b)?

[perezju, 2017-09-19 13:59:04]

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/job.py (right):

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:248: # This loop adds Changes to the
_changes list while looping through it.
On 2017/09/19 13:54:52, perezju wrote:
> Actually, it doesn't. Because of "tuple" you're now iterating through a copy
of
> the original list. Which is fine!

Ah, ignore both my previous comments. I just noted why you need to yield the
index, and your comment here is indeed important.

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:260: self.AddChange(midpoint, index)
Do we really need AddChange to be a public method?

[dtu, 2017-09-19 18:21:54]

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/job.py (right):

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:248: # This loop adds Changes to the
_changes list while looping through it.
On 2017/09/19 13:59:04, perezju wrote:
> On 2017/09/19 13:54:52, perezju wrote:
> > Actually, it doesn't. Because of "tuple" you're now iterating through a copy
> of
> > the original list. Which is fine!
> 
> Ah, ignore both my previous comments. I just noted why you need to yield the
> index, and your comment here is indeed important.

Yep, the index is needed for the insertion. Since inserting elements
simultaneously uses and modifies the array indices, it's important to go
backwards.

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:260: self.AddChange(midpoint, index)
On 2017/09/19 13:59:04, perezju wrote:
> Do we really need AddChange to be a public method?

One of the original interactivity feature plans was for the user to be able to
guide the course of the bisect by saying "please run this commit specifically."
While we might not expose something as low-level as AddChange() to the user, we
will probably want to use it to implement that kind of functionality.

https://codereview.chromium.org/3013153002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/job.py:288: yield index, change_b
On 2017/09/19 13:54:52, perezju wrote:
> Looking at what clients actually want, maybe it would be neater if you yield
> (change_a, change_b)?

We will also need the index for AsDict(), below on line 316.

[dtu, 2017-09-19 18:22:00]

The CQ bit was checked by dtu@chromium.org

[commit-bot: I haz the power, 2017-09-19 18:22:10]

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, 2017-09-19 18:53:39]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1505845320538760, "parent_rev":
"ab30bb20a857ea0136d8bf5ae449ef2b58690658", "commit_rev":
"1a128a7a63b4596a73c063d053572a0120ed6e73"}

[commit-bot: I haz the power, 2017-09-19 18:53:42]

Description was changed from

==========
[pinpoint] Use _JobState.Differences() in _JobState.Explore().
==========

to

==========
[pinpoint] Use _JobState.Differences() in _JobState.Explore().

Review-Url: https://chromiumcodereview.appspot.com/3013153002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-09-19 18:53:43]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...