Bug 104072 - nrwt: remove --no-record-results
Summary: nrwt: remove --no-record-results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 104064
Blocks: 103824 104158
  Show dependency treegraph
 
Reported: 2012-12-04 17:30 PST by Dirk Pranke
Modified: 2012-12-05 17:25 PST (History)
4 users (show)

See Also:


Attachments
Patch (24.11 KB, patch)
2012-12-04 17:31 PST, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-12-04 17:30:46 PST
nrwt: remove --no-record-results
Comment 1 Dirk Pranke 2012-12-04 17:31:48 PST
Created attachment 177618 [details]
Patch
Comment 2 Ojan Vafai 2012-12-05 14:05:17 PST
Comment on attachment 177618 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:376
> +                                                num_workers=1, retrying=True)

nit: Don't we typically only indent things like this 4 spaces?
Comment 3 Dirk Pranke 2012-12-05 14:26:36 PST
Comment on attachment 177618 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:376
>> +                                                num_workers=1, retrying=True)
> 
> nit: Don't we typically only indent things like this 4 spaces?

I think it's acceptable to either do 4 spaces or line up w/ the opening paren.
Comment 4 Ryosuke Niwa 2012-12-05 15:06:35 PST
(In reply to comment #3)
> (From update of attachment 177618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177618&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:376
> >> +                                                num_workers=1, retrying=True)
> > 
> > nit: Don't we typically only indent things like this 4 spaces?
> 
> I think it's acceptable to either do 4 spaces or line up w/ the opening paren.

No, WebKit style is to always indent by 4 spaces.
Comment 5 Dirk Pranke 2012-12-05 15:24:32 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 177618 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177618&action=review
> > 
> > >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:376
> > >> +                                                num_workers=1, retrying=True)
> > > 
> > > nit: Don't we typically only indent things like this 4 spaces?
> > 
> > I think it's acceptable to either do 4 spaces or line up w/ the opening paren.
> 
> No, WebKit style is to always indent by 4 spaces.

Hm. PEP-8 style is to line up with the paren, and we generally follow PEP-8 in Python code, as noted in http://trac.webkit.org/wiki/PythonGuidelines . It doesn't look like the main coding style web page ( http://www.webkit.org/coding/coding-style.html ) actually says anything about multi-line function calls one way or another, for any language. And, I would bet that more python code follows PEP-8 in this regard than WK style.

That said, I don't really care. Looks like both pep8 and pylint don't complain about this either way. I will fix this case.
Comment 6 Dirk Pranke 2012-12-05 15:25:58 PST
Committed r136768: <http://trac.webkit.org/changeset/136768>
Comment 7 Ryosuke Niwa 2012-12-05 15:32:57 PST
(In reply to comment #5)
> Hm. PEP-8 style is to line up with the paren, and we generally follow PEP-8 in Python code, as noted in http://trac.webkit.org/wiki/PythonGuidelines . It doesn't look like the main coding style web page ( http://www.webkit.org/coding/coding-style.html ) actually says anything about multi-line function calls one way or another, for any language. And, I would bet that more python code follows PEP-8 in this regard than WK style.

"The indent size is 4 spaces." in the indentation section refers to ALL indentations.
Comment 8 Dirk Pranke 2012-12-05 15:51:15 PST
(In reply to comment #7)
> (In reply to comment #5)
> > Hm. PEP-8 style is to line up with the paren, and we generally follow PEP-8 in Python code, as noted in http://trac.webkit.org/wiki/PythonGuidelines . It doesn't look like the main coding style web page ( http://www.webkit.org/coding/coding-style.html ) actually says anything about multi-line function calls one way or another, for any language. And, I would bet that more python code follows PEP-8 in this regard than WK style.
> 
> "The indent size is 4 spaces." in the indentation section refers to ALL indentations.

If you say so. It would be nice if it mentioned this case explicitly, because I would not have (and didn't) make that leap. Also, it's not obvious that that page does (or should) apply to python code.
Comment 9 Ryosuke Niwa 2012-12-05 15:54:20 PST
(In reply to comment #8)
> If you say so. It would be nice if it mentioned this case explicitly, because I would not have (and didn't) make that leap.

Feel free to submit a patch but this had never come up on C++ side of things. I think the part of confusion comes from old python code that followed Google style guide instead.
Comment 10 Dirk Pranke 2012-12-05 16:37:58 PST
(In reply to comment #9)
> (In reply to comment #8)
> > If you say so. It would be nice if it mentioned this case explicitly, because I would not have (and didn't) make that leap.
> 
> Feel free to submit a patch but this had never come up on C++ side of things. I think the part of confusion comes from old python code that followed Google style guide instead.

No, this is coming up because I write code that follows PEP-8 style even for new code :).
Comment 11 Ryosuke Niwa 2012-12-05 17:06:54 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > If you say so. It would be nice if it mentioned this case explicitly, because I would not have (and didn't) make that leap.
> > 
> > Feel free to submit a patch but this had never come up on C++ side of things. I think the part of confusion comes from old python code that followed Google style guide instead.
> 
> No, this is coming up because I write code that follows PEP-8 style even for new code :).

But PEP8 doesn’t prevent indentations to be always 4 spaces.
Comment 12 Dirk Pranke 2012-12-05 17:09:52 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > If you say so. It would be nice if it mentioned this case explicitly, because I would not have (and didn't) make that leap.
> > > 
> > > Feel free to submit a patch but this had never come up on C++ side of things. I think the part of confusion comes from old python code that followed Google style guide instead.
> > 
> > No, this is coming up because I write code that follows PEP-8 style even for new code :).
> 
> But PEP8 doesn’t prevent indentations to be always 4 spaces.

PEP-8 (the actual standard) says that function arguments should be aligned with the paren, and that left-indenting is wrong. pep8.py (the tool) and pylint do not seem to enforce that.

http://www.python.org/dev/peps/pep-0008/#indentation
Comment 13 Ryosuke Niwa 2012-12-05 17:16:54 PST
(In reply to comment #12)
> (In reply to comment #11)
> > But PEP8 doesn’t prevent indentations to be always 4 spaces.
> 
> PEP-8 (the actual standard) says that function arguments should be aligned with the paren, and that left-indenting is wrong. pep8.py (the tool) and pylint do not seem to enforce that.
> 
> http://www.python.org/dev/peps/pep-0008/#indentation

See examples:
# More indentation included to distinguish this from the rest.
def long_function_name(
        var_one, var_two, var_three,
        var_four):
    print(var_one)

and

# Extra indentation is not necessary.
foo = long_function_name(
  var_one, var_two,
  var_three, var_four)

are both acceptable.
Comment 14 Dirk Pranke 2012-12-05 17:25:13 PST
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > But PEP8 doesn’t prevent indentations to be always 4 spaces.
> > 
> > PEP-8 (the actual standard) says that function arguments should be aligned with the paren, and that left-indenting is wrong. pep8.py (the tool) and pylint do not seem to enforce that.
> > 
> > http://www.python.org/dev/peps/pep-0008/#indentation
> 
> See examples:
> # More indentation included to distinguish this from the rest.
> def long_function_name(
>         var_one, var_two, var_three,
>         var_four):
>     print(var_one)
> 
> and
> 
> # Extra indentation is not necessary.
> foo = long_function_name(
>   var_one, var_two,
>   var_three, var_four)
> 
> are both acceptable.

Okay, there are some cases where you not aligning w/ the left paren appear to be okay, but the line ojan commented on has variables on the first line and the second (and only) subsequent line: it matches the first "no" example, not the second "yes" example or the "optional" example.