[Predator] Make ``CallStackDetector`` return a namedtuple.

[Predator] Make ``CallStackDetector`` return a namedtuple.

BUG=677321
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2607813002
Committed: https://chromium.googlesource.com/infra/infra/+/625719640848f6141d0c6e261fd743fb15b005ed

[Sharu Jiang, 2016-12-28 20:58:42]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2016-12-28 20:59:33]

katesonia@chromium.org changed reviewers:
+ stgao@chromium.org, wrengr@chromium.org

[Sharu Jiang, 2016-12-28 20:59:34]

PTAL.

[Sharu Jiang, 2016-12-28 20:59:49]

Description was changed from

==========
[Predator] Make ``CallStackDetector`` returns a namedtuple.

BUG=677321
==========

to

==========
[Predator] Make ``CallStackDetector`` returns a namedtuple.

BUG=677321
TBR=stgao@chromium.org
==========

[wrengr, 2016-12-29 20:13:03]

Code-wise, looks good. Though I have a question...:

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/callstack_detectors.py (right):

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:14: namedtuple('StartOfCallStack',
I'd put this on the previous line, to match the style of all the other
namedtuples

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:17: 
Shouldn't there be a header word here? like "Properties:" or "Attributes:" or
something?

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:25: 
Maybe add a __new__ constructor, so that the metatata can be made optional
(since most callers of the constructor pass the empty dict)

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:27: return
('StartOfCallStack(priority = %d, format_type = %d, '
Use '%s' and self.__class__.__name__ instead of repeating the class name; to DRY
and to make future refactoring easier.

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:37: # TODO(crbug.com/677321) Make
the return to be a namedtuple.
the todo is redundant now

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:38: def __call__(self, line,
flags=None):
+1 to using __call__ :)

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/chromecrash_parser.py (right):

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/chromecrash_parser.py:43:
metadata=start_of_callstack.metadata)
Since we're just passing all the parts through, why not have the stack_detector
return the new CallStackBuffer (or None) directly?

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/clusterfuzz_parser.py (right):

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/clusterfuzz_parser.py:117:
metadata=start_of_callstack.metadata)
ditto.

[Sharu Jiang, 2016-12-29 21:40:15]

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/callstack_detectors.py (right):

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:14: namedtuple('StartOfCallStack',
On 2016/12/29 20:13:02, wrengr wrote:
> I'd put this on the previous line, to match the style of all the other
> namedtuples

Actually that's a wrong indentation. Didn't notice before...
https://google.github.io/styleguide/pyguide.html#Indentation

Either:
       # Aligned with opening delimiter
       foo = long_function_name(var_one, var_two,
                                var_three, var_four)

Or:
       # 4-space hanging indent; nothing on first line
       foo = long_function_name(
           var_one, var_two, var_three,
           var_four)

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:17: 
On 2016/12/29 20:13:02, wrengr wrote:
> Shouldn't there be a header word here? like "Properties:" or "Attributes:" or
> something?

Done.

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:25: 
On 2016/12/29 20:13:02, wrengr wrote:
> Maybe add a __new__ constructor, so that the metatata can be made optional
> (since most callers of the constructor pass the empty dict)

Done.

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:27: return
('StartOfCallStack(priority = %d, format_type = %d, '
On 2016/12/29 20:13:02, wrengr wrote:
> Use '%s' and self.__class__.__name__ instead of repeating the class name; to
DRY
> and to make future refactoring easier.

Done.

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:37: # TODO(crbug.com/677321) Make
the return to be a namedtuple.
On 2016/12/29 20:13:02, wrengr wrote:
> the todo is redundant now

Oops, should have cleaned this.

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/chromecrash_parser.py (right):

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/chromecrash_parser.py:43:
metadata=start_of_callstack.metadata)
On 2016/12/29 20:13:02, wrengr wrote:
> Since we're just passing all the parts through, why not have the
stack_detector
> return the new CallStackBuffer (or None) directly?

I think it would be more clear that the ``stack_detector`` only detects the
``StartOfCallstack``, and let the ``Parser`` takes care of the whole
``stack_buffer`` (including creating the ``stack_buffer`` based
``StartOfCallStack`` and adding all the frames.)

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/clusterfuzz_parser.py (right):

https://codereview.chromium.org/2607813002/diff/40001/appengine/findit/crash/...
appengine/findit/crash/clusterfuzz_parser.py:117:
metadata=start_of_callstack.metadata)
On 2016/12/29 20:13:03, wrengr wrote:
> ditto.

As above

[Sharu Jiang, 2016-12-29 22:21:29]

Description was changed from

==========
[Predator] Make ``CallStackDetector`` returns a namedtuple.

BUG=677321
TBR=stgao@chromium.org
==========

to

==========
[Predator] Make ``CallStackDetector`` return a namedtuple.

BUG=677321
TBR=stgao@chromium.org
==========

[wrengr, 2016-12-29 23:36:27]

lgtm with nit

https://codereview.chromium.org/2607813002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/callstack_detectors.py (right):

https://codereview.chromium.org/2607813002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:29: language_type, metadata)
Should ``metadata or {}`` so that it's a dict rather than NoneType

[Sharu Jiang, 2016-12-30 00:18:01]

https://codereview.chromium.org/2607813002/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/callstack_detectors.py (right):

https://codereview.chromium.org/2607813002/diff/80001/appengine/findit/crash/...
appengine/findit/crash/callstack_detectors.py:29: language_type, metadata)
On 2016/12/29 23:36:27, wrengr wrote:
> Should ``metadata or {}`` so that it's a dict rather than NoneType

Done.

[Sharu Jiang, 2016-12-30 00:18:13]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-12-30 00:18:14]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2607813002/#ps100001
(title: "Rebase and fix nit.")

[commit-bot: I haz the power, 2016-12-30 00:18:21]

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-12-30 00:27:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483057093807830,
"parent_rev": "389001b9706230000fbba929fde751a7bd8a70a6", "commit_rev":
"625719640848f6141d0c6e261fd743fb15b005ed"}

[commit-bot: I haz the power, 2016-12-30 00:27:24]

Description was changed from

==========
[Predator] Make ``CallStackDetector`` return a namedtuple.

BUG=677321
TBR=stgao@chromium.org
==========

to

==========
[Predator] Make ``CallStackDetector`` return a namedtuple.

BUG=677321
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2607813002
Committed:
https://chromium.googlesource.com/infra/infra/+/625719640848f6141d0c6e261fd74...
==========

[commit-bot: I haz the power, 2016-12-30 00:27:25]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/625719640848f6141d0c6e261fd74...