Closing paren indentation should be more flexible #103

Closed
mapleoin opened this Issue Jul 7, 2012 · 27 comments

Projects

None yet

8 participants

@mapleoin

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
Python Code Quality Authority member
@warsaw
@sigmavirus24
Python Code Quality Authority member

Thank you!

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

@warsaw
@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 #103 (comment)
It does not seem so wrong...

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

@warsaw
@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
@sigmavirus24
Python Code Quality Authority member

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

@feth feth added a commit to majerteam/CaeMobile that referenced this issue Nov 28, 2013
@feth feth pep8 on main.py
actually, from now on, i'll --ignore=E123.
ref: PyCQA/pycodestyle#103 (comment)
7c09ddb
@florentx

This was fixed in 1.4.6

@florentx florentx closed this Mar 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment