Bug 104072 – nrwt: remove --no-record-results

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: remove --no-record-results

[Dirk Pranke]

Created attachment 177618 [details]
Patch

[Ojan Vafai]

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?

[Dirk Pranke]

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.

[Ryosuke Niwa]

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

[Dirk Pranke]

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

[Dirk Pranke]

Committed r136768: <http://trac.webkit.org/changeset/136768>

[Ryosuke Niwa]

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

[Dirk Pranke]

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

[Ryosuke Niwa]

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

[Dirk Pranke]

(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 :).

[Ryosuke Niwa]

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

[Dirk Pranke]

(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

[Ryosuke Niwa]

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

[Dirk Pranke]

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