RESOLVED FIXED 104072
nrwt: remove --no-record-results
https://bugs.webkit.org/show_bug.cgi?id=104072
Summary nrwt: remove --no-record-results
Dirk Pranke
Reported 2012-12-04 17:30:46 PST
nrwt: remove --no-record-results
Attachments
Patch (24.11 KB, patch)
2012-12-04 17:31 PST, Dirk Pranke
ojan: review+
Dirk Pranke
Comment 1 2012-12-04 17:31:48 PST
Ojan Vafai
Comment 2 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?
Dirk Pranke
Comment 3 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.
Ryosuke Niwa
Comment 4 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.
Dirk Pranke
Comment 5 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.
Dirk Pranke
Comment 6 2012-12-05 15:25:58 PST
Ryosuke Niwa
Comment 7 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.
Dirk Pranke
Comment 8 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.
Ryosuke Niwa
Comment 9 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.
Dirk Pranke
Comment 10 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 :).
Ryosuke Niwa
Comment 11 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.
Dirk Pranke
Comment 12 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
Ryosuke Niwa
Comment 13 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.
Dirk Pranke
Comment 14 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.
Note You need to log in before you can comment on or make changes to this bug.