E125 overreaches pep8 #126

Closed
jeblair opened this Issue Sep 26, 2012 · 10 comments

7 participants

@jeblair

In pep8 version 1.3.3, E125 will erroneously alert on the following code:

if (foo == bar and
    bar == foo):
    qux = foo

citing:

E125 continuation line does not distinguish itself from next logical line

However, that sentence is a clause that only applies to hanging indentation, not to vertically aligned lines joined inside parenthesis. Here is the relevant statement from pep8:

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent. When using a hanging indent
the following considerations should be applied; there should be no
arguments on the first line and further indentation should be used
to clearly distinguish itself as a continuation line.

Note that the additional considerations do not apply to anything but a hanging indent.

@slacy

Agreed, and I'm going to have to globally disable E125 because of this. :(

@florentx

The PEP8 sentence is about readability. Even if it does not forbid strictly this case, it has the same consequence on readability.

The above example might be rewritten like:

if (foo == bar and
        bar == foo):
    qux = foo

or (a preference for it)

if ((foo == bar and
     bar == foo)):
    qux = foo

or (but I don't like)

if (
    foo == bar and
    bar == foo
):
    qux = foo
@dev97

It's unfortunate coincidence that "if<space>(" takes exactly 4 spaces. But indenting code like in first snippet is very very ugly. Second snippet resembles the way to hush gcc when assignment is intentional inside condition [if ((c = get_token())) ...], but in case with gcc it's for tricky code and acts as a warn sign. But to use it for every multiline condition that's too much. I don't have words about third snippet ;-)

Fortunately E125 can be globally disabled in ~/.config/pep8.

Nevertheless even the PEP8 reference code (see class Rectangle, if statement) explicitly shows that traditional indentation (when continuation indented relative to opening bracket) is valid.

@il86be

I would suggest to indent the conditional block relative to the last line of the condition:

if (foo == bar and
    bar == foo):
        qux = foo

but pep8 1.3.3 still complains on such code. It seems that it does not even look at the "next logical line" when it reports E125

@florentx

no this indentation will not be supported by pep8, because the recommended indentation is 4 spaces per level.

And I totally agree with this rule. If you use 8 spaces like the above example, it adds some confusion: do you indent to 4 or 8 in the elif or the else clause?
And if you change the conditional later, you'll have to re-indent the whole paragraph in this case.

By the way, since version 1.4.2 I've implemented the proposal of issue #151 to accept this form too:

if foo == bar and \
   bar == foo:
    qux = foo

It is an alternative to the other proposals (copied from a previous message):

if (foo == bar and
        bar == foo):
    qux = foo
if ((foo == bar and
     bar == foo)):
    qux = foo
@jeblair

Hi,

Can we get back to the original topic of this bug? I did not suggest any alternate indentation. Merely that E125 is not supported by the text in the pep 8 specification that it claims to be. And dev97 helpfully pointed to an example in pep 8 that explicitly contradicts E125.

Following the specification is important because it's what editors and other tools are written to use. Especially since this tools is named after the specification, it should follow it.

Please adjust the test for E125 to match the specification, thanks.

@florentx

The proposed branch splits E125 error. It allows to skip E129 if it matches your preferences.

@john-aws

Using elif in the same way does not cause E125, presumably because it's longer resulting in distinct indentation for the elif arm.

if (index1 == 5 and
    index2 == 3): # raises E125
    pass
elif (index1 == 6 and
      index2 == 1): # clean
    pass
@thedrow thedrow referenced this issue in nose-devs/nose2 Jun 8, 2013
Merged

Fixed PEP8 style errors using autopep8. #88

@akaihola

For future reference, the Stack Overflow question Python style: multiple-line conditions in IFs has a comprehensive set of style suggestions for long if conditions.

I know this ticket isn't about alternatives but strictly following the spec, but this link is worth mentioning since this page is among the top search results for "pep8 continuation line".

@dhellmann dhellmann pushed a commit to dhellmann/oslo-incubator that referenced this issue Oct 31, 2013
@markmc markmc Some nitpicky securemessage cleanups
Remove redundant parenthesis and use a consistent commenting style.

Some redundant parenthesis are retained to avoid this pep8 silliness:

  PyCQA/pycodestyle#126

i.e. to avoid E125 we have the choice of:

        if (md['source'] != source or
                md['destination'] != target or
                md['expiration'] < time.time()):
            raise InvalidEncryptedTicket(md['source'], md['destination'])

or:

        if ((md['source'] != source or
             md['destination'] != target or
             md['expiration'] < time.time())):
            raise InvalidEncryptedTicket(md['source'], md['destination'])

the latter is my preference.

Change-Id: Ic890bfc787a17fc77821e01e586806d1b558d530
848c4d5
@timgraham timgraham referenced this issue in django/django Nov 19, 2013
Closed

Fixed E125 pep8 warnings #1930

@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this issue Nov 26, 2013
Jenkins Updated openstack/openstack
Project: openstack/nova  fa024289be4114f6b31078721e8608b801776da2
null
Don't gate on E125

E125 overreaches on what pep8 specifies. See
PyCQA/pycodestyle#126

In practice E125 can be a pain as compliance requires
manual indenting if using emacs and refactoring tools
don't always get it right.

There's a bit of a discussion about it here (on a tempest
changeset):

https://review.openstack.org/#/c/36788/

Change-Id: Ic73ab3c4a47f33de9145e0c7db2d8674230c2fe0
818e41c
@openstack-gerrit openstack-gerrit pushed a commit to openstack/nova that referenced this issue Nov 26, 2013
Chris Yeoh Don't gate on E125
E125 overreaches on what pep8 specifies. See
PyCQA/pycodestyle#126

In practice E125 can be a pain as compliance requires
manual indenting if using emacs and refactoring tools
don't always get it right.

There's a bit of a discussion about it here (on a tempest
changeset):

https://review.openstack.org/#/c/36788/

Change-Id: Ic73ab3c4a47f33de9145e0c7db2d8674230c2fe0
48a0fdc
@n0ano n0ano pushed a commit to n0ano/gantt that referenced this issue Dec 11, 2013
Chris Yeoh Don't gate on E125
E125 overreaches on what pep8 specifies. See
PyCQA/pycodestyle#126

In practice E125 can be a pain as compliance requires
manual indenting if using emacs and refactoring tools
don't always get it right.

There's a bit of a discussion about it here (on a tempest
changeset):

https://review.openstack.org/#/c/36788/

Change-Id: Ic73ab3c4a47f33de9145e0c7db2d8674230c2fe0
f16b987
@florentx florentx removed the opinion label Mar 25, 2014
@florentx

In next version of pep8 you can add E129 to the ignore list: 3f340b8.

@florentx florentx closed this Mar 25, 2014
@florentx florentx added the feature label Mar 25, 2014
@florentx florentx added this to the 1.5.0 milestone Mar 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment