[webkit-reviews] review granted: [Bug 184039] webkitpy: Implement coredumpctl support on linux : [Attachment 337700] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Thibault Saunier
<tsaunier at gnome.org>'s request for review:
Bug 184039: webkitpy: Implement coredumpctl support on linux
https://bugs.webkit.org/show_bug.cgi?id=184039

Attachment 337700: Patch

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




--- Comment #19 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 337700
  --> https://bugs.webkit.org/attachment.cgi?id=337700
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337700&action=review

Thank you Thibault for iterating on this patch. The patch looks sane to me.

> 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.

> 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.