Pycodestyle: Closing paren indentation should be more flexible

[iartarisi]

From what I could see, there is no advice in PEP-8 to discourage using closing parens on the same line as the last item in a list, e.g.

foo = {
       "bar": (
           "baz",
           "qux"
           )  # <-- this is discouraged
       }  # <-- as is this (n.b. it is one column to the right)

This is the default indentation style in emacs.

pep8 says: _E123 closing bracket does not match indentation of opening bracket's line_

[avkoval]

yes, I also can not find the definition of this rule in original pep8: http://www.python.org/dev/peps/pep-0008/
What I do to avoid this E123: I am moving the closing bracket to the previous line. It looks less readable though..
in my .emacs.d. setup I can ignore various kinds of errors. So may be good advice for mapleoin to look at: http://avk-emacs.hg.public.halogen-dg.com/hgweb.cgi/file/4dfe5f008ad9/pyflymake.py

[florentx]

thank you for reporting this.
I agree that this rule is not strictly enforced by the PEP 8.

For the justification of E123, I propose to refer to the initial pull request #64 by @samv (the error codes were renumbered when the patch was merged):

All of these errors are directly justifiable from text and examples in
the PEP8 document, and some examples are directly copied from PEP8. The
exception to this is E123, which the PEP8 standard appears neutral on
but nonetheless I feel is intrinsically related with the rules which are
defined here; to not enforce it would permit multiple valid indent
levels for closing brackets which start lines, leading to ambiguity.
All the PEP8 examples leave the closing bracket on the previous line,
avoiding this question. This E123 rule agrees with K&R, KNF, Linux,
this VIM auto–indent script:

http://www.vim.org/scripts/script.php?script_id=974

and the fix to python-mode achieved by the elisp fragment on:

http://stackoverflow.com/revisions/5361478/2

See testsuite/E12.py for many examples.

[warsaw]

pep8 is simply wrong here, and it's highly annoying ;) Take for example a common construct such as all:

__all__ = [
    'TestFoo',
    'TestGoo',
    'TestHoo',
    ]

You'll get an E123 on this construct, which can be "fixed" by dedenting the closing brace 4 spaces, so that it lines up under the first underscore on the first line. This is both wrong and ugly, and breaks defaults in e.g. python-mode. As co-author of PEP 8, trust me on this one :).

[sigmavirus24]

Hey Uncle Barry,

Does the same hold for dictionary definitions (with {}s), tuple definitions, and function calls?

[warsaw]

On Apr 15, 2013, at 12:52 PM, Ian Cordasco wrote:

Does the same hold for dictionary definitions (with {}s), tuple definitions,
and function calls?

Yep! :) These are all preferred forms:

def foo():
mydict = {
'a': 1,
'b': 2,
'c': 3,
}
mytuple = (
'hello',
'goodbye',
'yes',
'no',
)
mylist = [
1, 2, 3,
4, 5, 6,
]
result = my_function_is_called_with_these_arguments(
1, 2, 3, 4, 5,
'hello',
'oops',
yes=1, no=0, maybe=-1,
)

[sigmavirus24]

Thank you!

For the time being, you could also do --ignore=E123 :)

[warsaw]

On Apr 15, 2013, at 02:06 PM, Ian Cordasco wrote:

For the time being, you could also do --ignore=E123 :)

Yep, that's what I'm doing! :)

Thanks!

[florentx]

Thank you for your clarification, @warsaw.
It was not obvious to me that this style is _"simply wrong"_ ... maybe because I'm not Dutch :)

I find it natural and legible to align the closing bracket with the start of the opening bracket's line.
This is something I find in various places, and I use it for years now.
When @samv proposed to include it within the E12* checks, I did not receive any objection.
Furthermore, the same style seems already adopted in various places: the rationale is explained here https://github.com/jcrocholl/pep8/issues/103#issuecomment-13432725
It does not seem so wrong...

Finally, I suggest you add --ignore W603,E123 to enjoy the pep8 tool with your preferences ;)

[warsaw]

On Apr 16, 2013, at 03:19 PM, Florent Xicluna wrote:

It was not obvious to me that this style is _"simply wrong"_ ... maybe
because I'm not Dutch :)

Me neither, and my Polish blood only serves me when the letters before FL are
FLU not BD. :)

So I asked Guido, and he said that PEP 8 should explicitly allow both forms.
I am going to update PEP 8 to say this.

I'm not sure what that means for the pep8 tool, but I think in light of that
it probably shouldn't prefer one style over the other. It's easy enough to
suppress E123 but then I don't get nice warnings when someone is using
something that isn't either style.

What about adding another error for the line-em-all-up style I prefer? Then
users would have the option to suppress one or the other as appropriate for
their project?

[florentx]

Thanks for the update.

I'm +1 for improving the behavior with --ignore E123 to report misindented closing parenthesis.
I'll have a look.

Adding another error might be confusing though, because both errors will be fully incompatible. If the user forget to exclude one of them, the code will never be compliant (unless the closing parenthesis is moved to the end of the previous line).
Or we need to add some safety check to prevent both errors being active at the same time...

[warsaw]

On Apr 19, 2013, at 03:02 AM, Florent Xicluna wrote:

Or we need to add some safety check to prevent both errors being active at
the same time...

Personally, I like this solution, but it's your project so you decide! :)

[sigmavirus24]

The issue as I see it is someone mixing the two styles in one file or code-base. PEP8 was meant to instill consistency. Would it perhaps be reasonable to add a Warning that the two styles have been mixed or would that be an unreasonable setting?

[warsaw]

On Apr 19, 2013, at 10:01 AM, Ian Cordasco wrote:

The issue as I see it is someone mixing the two styles in one file or
code-base. PEP8 was meant to instill consistency. Would it perhaps be
reasonable to add a Warning that the two styles have been mixed or would that
be an unreasonable setting?

Yes, IMO it would be reasonable to warn if the two styles were mixed.

[reinout]

pep8 should instill consistency, but mostly with the pep 8 standard. Are there other spots in the code where two alternatives are valid and does pep8 check that only one style is used?

Simply allowing both alternatives looks fine enough for me.

[florentx]

This addition of PEP-8 is fairly recent, @reinout (April 2013).
I plan to come with a proposal before next release.

[florentx]

So we have 3 user preferences which comply with PEP-8:

• match indentation with indentation of opening bracket's line (pep8 1.4.5 behavior)
• hang indentation of the closing bracket the same as indentation of the previous line
• or accept both styles in the same project

The first preference is the current behavior, and it triggers E123 when the closing bracket is hanging.
The third preference is accomplished with --ignore E123 (fixed with previous commit).

For the second preference, I propose an additional switch --hang-indent which switches the behavior. If the flag is set, the error E123 is no longer reported, but error E133 closing bracket is missing indentation is reported instead.

Sample file _"test_issue103.py"_:

my_list = [
    1, 2, 3,
    4, 5, 6,
]

result = some_function_that_takes_arguments(
    'a', 'b', 'c',
    'd', 'e', 'f',
    )

---

Demonstrate the three preferences:

$ # Preference (1)
$ pep8 --show-source test_issue103.py 
test_issue103.py:9:5: E123 closing bracket does not match indentation of opening bracket's line
    )
    ^
$ 
$ # Preference (2)
$ pep8 --show-source --hang-closing test_issue103.py 
test_issue103.py:4:1: E133 closing bracket is missing indentation
]
^
$ 
$ # Preference (3)
$ pep8 --show-source --ignore E123 test_issue103.py 
$ 

[warsaw]

On Apr 27, 2013, at 03:06 AM, Florent Xicluna wrote:

For the second preference, I propose an additional switch --hang-indent
which switches the behavior. If the flag is set, the error E123 is no
longer reported, but error E133 closing bracket is missing indentation is
reported instead.

I like this. Is it possible to set --hang-indent in a configuration file,
e.g. setup.cfg?

[florentx]

Is it possible to set --hang-indent in a configuration file, e.g. setup.cfg?

Yes, it's in the patch.

[reinout]

I don't want to have to set a configuration option in 50 projects to get pep8.py to stop reporting a false error. Neither do I want to tell 12 colleagues to set a global option.

So: I'd be careful to provide options to keep pep8.py from tripping over valid code. Pep8-compliant code should, by default, not raise an error. An option to enforce either possibility as the only allowed one is fine.

[florentx]

Since the activation of E12 checks in version 1.3, there's an error E123 which is reported if the closing bracket does not match indentation of the opening bracket's line.

@reinout do you suggest we add E123 to the DEFAULT_IGNORE list, in order to accept a mix of both styles by default?
It seems a reasonable choice.

[reinout]

Accept both by default, yes. Pep8.py should ensure pep8-compliant code, but it should not go further than pep8.

I think this is one of the few cases where there is more than one option in the standard. Anyway, having an option to, by choice, enforce one of the styles is fine. To me, it is fine to only optionally enforce the variant that's currently the default.

[andreas-roehler]

Hi all,

really think closing bracket should not match indentation of opening bracket's line, unless at col zero, i.e. if composing a statement by themselves.

AFAIU in general a closing points at the form opened, matches its indent.
The closing brace/bracket/paren must not point at the form opened at indent, but at the from started at the right-hand-side. RHS makes a difference, which should be visible.

[florentx]

@andreas-roehler could you give a simple example of what you advocate?
Thank you.

[andreas-roehler]

Here an example with code from issue 19
https://gist.github.com/andreas-roehler/5504855

Best regards,
Andreas

[florentx]

Thanks for the example.

Personally I have a preference reading this style:

foo = {
    "bar": (
         "baz",
         "qux",
    ),
}

And there are defendants for both styles of formatting, of course.

However the bikeshedding is over, because both styles are now explicitly allowed by the PEP-8 style guide. The next version of pep8 will gain flexibility on it, as explained in previous comments.

Changes in this branch:

* Correctly report other E12 errors when E123 is ignored. (Issue #103)

* New option ``--hang-closing`` to switch to the alternative style of
  closing bracket indentation for hanging indent.  Add error E133 for
  closing bracket which is missing indentation. (Issue #103)

* Accept both styles of closing bracket indentation for hanging indent.
  Do not report error E123 in the default configuration. (Issue #103)

[eode]

Thank you for fixing this to accept both styles, since both styles are valid PEP8. Since PEP8 does promote consistency, one thing you could do is to warn about mixed styles (if a file is using both styles).

[florentx]

This was fixed in 1.4.6