[webkit-reviews] review requested: [Bug 184039] webkitpy: Implement coredumpctl support on linux : [Attachment 339979] webkitpy: Implement coredumpctl support on linux

[bugzilla-daemon at webkit.org]

Thibault Saunier <tsaunier at gnome.org> has asked  for review:
Bug 184039: webkitpy: Implement coredumpctl support on linux
https://bugs.webkit.org/show_bug.cgi?id=184039

Attachment 339979: webkitpy: Implement coredumpctl support on linux

https://bugs.webkit.org/attachment.cgi?id=339979&action=review




--- Comment #20 from Thibault Saunier <tsaunier at gnome.org> ---
Created attachment 339979

  --> https://bugs.webkit.org/attachment.cgi?id=339979&action=review

webkitpy: Implement coredumpctl support on linux

(In reply to Daniel Bates from comment #19)
> Comment on attachment 337700 [details]
>
> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:77
> > +		     for datestr in re.findall(r'Timestamp: .*(\d\d\d\d-\d+-\d+
\d+:\d+:\d+)', info):
> 
> I do not see the need to abbreviate the word “string” and “date string” is
> two words. For your consideration I suggest that we name this variable
> timestamp given that the we are matching against the extracted “Timestamp”
> values. On another note, we can simplify this regular expression by using
> repetition syntax, \d{x}, where x is the number of repetitions: ‘Timestamp
> .*(\d{4}-\d+-\d+ \d+:\d+:\d+’. We could simplify this further using
> repetition syntax and non-capture groups though I am unclear whether that
> would hurt readability.

Updated to regex to use repetition syntax.

> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:107
> > +	     # Poor man which, ignore any failure.
> 
> For your consideration I suggest using manual page syntax when referencing
> another program name. So, “which”  => “which(1)”. This makes it
> straightforward for a reader to identify a command from an English word.

Done.