Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E125 overreaches pep8 #126

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

E125 overreaches pep8 #126

jeblair opened this issue Sep 26, 2012 · 10 comments
Labels
Milestone

Comments

@jeblair
Copy link

jeblair commented Sep 26, 2012

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
Copy link
Contributor

slacy commented Oct 1, 2012

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

@florentx
Copy link
Contributor

florentx commented Dec 9, 2012

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
Copy link

dev97 commented Jan 19, 2013

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
Copy link

il86be commented Mar 7, 2013

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
Copy link
Contributor

florentx commented Mar 7, 2013

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
Copy link
Author

jeblair commented Mar 7, 2013

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
Copy link
Contributor

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

@john-aws
Copy link

john-aws commented Jun 6, 2013

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

@akaihola
Copy link

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

openstack-gerrit pushed a commit to openstack/openstack that referenced this issue Nov 26, 2013
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
openstack-gerrit pushed a commit to openstack/nova that referenced this issue Nov 26, 2013
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
n0ano pushed a commit to n0ano/gantt that referenced this issue Dec 11, 2013
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
@florentx florentx removed the opinion label Mar 25, 2014
@florentx
Copy link
Contributor

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

mika pushed a commit to sipwise/jenkins-job-builder that referenced this issue Apr 29, 2024
However due to an upstream bug[1] we ignore E125 for now.

[1] PyCQA/pycodestyle#126

Change-Id: I75337d9194156580cc66666aed9a5bc2fd5d4e15
Signed-off-by: Paul Belanger <paul.belanger@polybeacon.com>
mika pushed a commit to sipwise/jenkins-job-builder that referenced this issue Apr 29, 2024
However due to an upstream bug[1] we ignore E125 for now.

[1] PyCQA/pycodestyle#126

Change-Id: I75337d9194156580cc66666aed9a5bc2fd5d4e15
Signed-off-by: Paul Belanger <paul.belanger@polybeacon.com>
mika pushed a commit to sipwise/jenkins-job-builder that referenced this issue Apr 29, 2024
However due to an upstream bug[1] we ignore E125 for now.

[1] PyCQA/pycodestyle#126

Change-Id: I75337d9194156580cc66666aed9a5bc2fd5d4e15
Signed-off-by: Paul Belanger <paul.belanger@polybeacon.com>
mika pushed a commit to sipwise/jenkins-job-builder that referenced this issue Apr 29, 2024
However due to an upstream bug[1] we ignore E125 for now.

[1] PyCQA/pycodestyle#126

Change-Id: I75337d9194156580cc66666aed9a5bc2fd5d4e15
Signed-off-by: Paul Belanger <paul.belanger@polybeacon.com>
mika pushed a commit to sipwise/jenkins-job-builder that referenced this issue Apr 29, 2024
However due to an upstream bug[1] we ignore E125 for now.

[1] PyCQA/pycodestyle#126

Change-Id: I75337d9194156580cc66666aed9a5bc2fd5d4e15
Signed-off-by: Paul Belanger <paul.belanger@polybeacon.com>
mika pushed a commit to sipwise/jenkins-job-builder that referenced this issue Apr 29, 2024
Patch Set 2: I would prefer that you didn't merge this

We already have a pep8 job running against JJB. You can run pep8 with tox: tox -epep8

Note that we purposefully disabled E125 due to what is considered an upstream bug:
openstack-infra/jenkins-job-builder@345fb6e
PyCQA/pycodestyle#126

If any work should be done on PEP8, I suggest we work on migrating to hacking instead which uses flake8. hacking is already used across OpenStack projects, we should use it too.

Patch-set: 2
Reviewer: Gerrit User 7156 <7156@4a232e18-c5a9-48ee-94c0-e04e7cca6543>
Label: Code-Review=-1
Label: Workflow=0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants