Update tox.ini files to ignore W503 and W504

[kzar]

Background

We use flake8 via tox to lint our Python code. We've configured tox to run both an old and a current version of flake8 automatically.

Older versions of flake8 had this rule:

W503 line break before binary operator

But never versions have this opposite rule instead:

W504 line break after binary operator

What to change

Update necessary tox.ini files to start ignoring both W503 and W504.

Notes

• ​We already started ignoring W503 in the cms repository.

[kzar]

Does anyone have an opinion about which rule we should ignore? I don't really care, but it was up to me I'd ignore W504. For example, I think that

def release_combination_is_possible(version, platforms, base_dir):

    ...

    def higher_tag_version(tag, version, platforms):
        return (compare_versions(tag[0], version) > 0 and
                set(tag[1:]).intersection(platforms))

    ...

looks nicer than

def release_combination_is_possible(version, platforms, base_dir):

    ...

    def higher_tag_version(tag, version, platforms):
        return (compare_versions(tag[0], version) > 0
                and set(tag[1:]).intersection(platforms))

    ...

[kzar]

After bringing this up in #python Tudor mentioned that he and Vasily decided to ignore W503, but only for the cms repository. All the more reason to tackle this once and for all IMO.

[rhowell]

I agree with kzar in the case shown above, the first looks nicer to me as well. But, for longer equations and code with more than 2-3 lines, the other way around seems more readable (see the examples linked below). It appears that the current PEP 8 recommendation is to put a binary operator after a line break:
​https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

[sebastian]

For ages PEP-8 was preferring to put binary operators before the line break. However, at some point this rule was ​relaxed, now stating:

In Python code, it is permissible to break before or after a binary operator, ...

Personally, I agree with kzar that breaking after the binary operator is more readable, also it is consistent with the majority of our existing Python code, and the practices we follow for other languages.

FWIW, flake8/pycodingstyles neither report W503 nor W504 by default. However, by using the ignore option we override the default ignores. But since flake8 3.6 there is a new option extend-ignore which we could use instead now, in order to add errors to be ignored without overriding the default ignores. Why would we want to be more strict than the default here, in particular if we cannot agree on one flavor?

[kzar]

Why would we want to be more strict than the default here, in particular if we cannot agree on one flavor?

Well, I don't think we do want to be more strict than default here.

I'm not super knowledgeable about flake8 etc, so if you have a proposal perhaps update the issue and let's see if everyone's on board with it?

[sebastian]

Follow-up discussion on IRC:

18:14:15 <•vasily> snoack: PEP-8 now recommends to break the line before binary operators in the new code, following Knuth, mathematics and stuff, so I think this style is preferred for new code. For existing code the switch might not be worth it though...
18:16:32 <•vasily> snoack: OTOH, I do see the advantage of having this taken care of at the level of flake8-eyeo.
18:18:52 <snoack> vasily: That is not acurrate. From PEP-8: "In Python code, it is permissible to break before or after a binary operator, as long as the convention is consistent locally. For new code Knuth's style is suggested."
18:18:52 <snoack> vasily: It's merely a suggestion, and in the same paragraph it says that both styles are permitted.
18:19:58 <•vasily> snoack: allright, no recommended, suggested. but if you look at the examples above it kind of looks like Knuth's style is preferred
18:23:02 <snoack> vasily: Also mind, we never would have this discussion, if we wouldn't have ended up overriding flake8's default ignores. By default flake8 is ignoring both, W503 and W504.
18:23:41 <•vasily> snoack: mm, yeah, that might also be a good option, actually
18:27:47 <•vasily> snoack: to me it seems that the choice between W503 and W504 is basically a matter of taste, however, it does seem like Python community is going forward with Knuth's style so trying to enforce the opposite would be a bit retrograde. But not enforcing anything (and given that our coding style still mentions PEP-8, so it's "suggested" bla bla stuff applies) seems ok.
18:29:43 <snoack> vasily: Do you have any reference, how the Python community is actually adapting Knuth's esoteric style? I do not remember having seen any Python code anywhere that actualy does use that style, before you stated to change the style in some of our projects.
18:31:12 <•vasily> snoack: PEP-8 suggests it... that's all I know.
18:33:11 <•vasily> snoack: I think I might have seen some code formatted this way in the wild but I don't remember where
18:34:43 <•vasily> snoack: however, my impression was that people don't put stuff into PEP-8 randomly, I suppose there's quite a bit of consensus if they actually changed the rule
18:53:28 <snoack> vasily: Sure, but I'm wondering if the change (https://hg.python.org/peps/rev/3857909d7956) was rather about relaxing the old rule, or about deliberately inverting it?
05:39:05 <•vasily> snoack: yeah, it's definitely a possible way to read it. The wording kind of suggests that Knuth's style is now preferred and some new projects might take the hint, but we don't have to. Should we just disable both W503 and W504 then?

It seems the consensus is to ignore both W503 and W504 (like flake8 does by default).

As for moving our configuration into flake8-eyeo (instead of duplicating the errors to be ignored/selected in each project's tox.ini), it is possible for a flake8 extension to configure arbitrary error codes to be ignored or to be selected. But the ​built-in default ignores take precedence. So not only W503 and W504 but also E121, E123, E126, E226, E24 and E704 would be ignored again.

[rhowell]

Should we mention something is the Style Guide about this? Maybe something like:

It is permissible to break a line before or after a binary operator, whichever is more readable. Generally, long lines that can be broken into 2-3 lines are more readable with the line break after the binary operator, and long boolean or mathematical equations (3 or more lines) are more readable with the line break before the binary operator. (And maybe we include the examples mentioned in this ticket.)

Also, it seems this ticket should have the 'What to change' modified and then be marked ready?

[sebastian]

That is already covered in PEP-8 which our code style is referring to.

Yes, the description of this issue isn't complete yet, that is why this issue isn't set to ready.

[rhowell]

After taking a closer look and discussing this change with Sebastian, we think it might be better to ignore W503 and W504 from the tox.ini files associated with each project. The main reason is that flake8 wasn't designed to support these ignores. Also, we already have a lot of configuration in tox.ini files that is duplicated across projects, and that seems to be where we have put these kinds of ignores in the past.

[kvas]

Replying to rhowell:

After taking a closer look and discussing this change with Sebastian, we think it might be better to ignore W503 and W504 from the tox.ini files associated with each project. The main reason is that flake8 wasn't designed to support these ignores. Also, we already have a lot of configuration in tox.ini files that is duplicated across projects, and that seems to be where we have put these kinds of ignores in the past.

Sounds good to me. Is there anything to do in this ticket then or shall it be closed?

[rhowell]

Sounds good to me. Is there anything to do in this ticket then or shall it be closed?

I was thinking I could use this ticket to clean up and standardize some of the tox.ini files that are already ignoring W503 or W504.

W503 is already ignored in cms and python-abp. W504 is already ignored in abpssembly, buildtools, and sitescripts, but some are using per-file-ignores, when both (W503 & W504) should probably just be general ignores.

[abpbot]

A commit referencing this issue has landed:
​Issue 7196 - Ignore both flake8 W503 and W504 in tox.ini

[abpbot]

A commit referencing this issue has landed:
​Issue 7196 - Ignore both flake8 W503 and W504 in tox.ini

[abpbot]

A commit referencing this issue has landed:
​Issue 7196 - Ignore both flake8 W503 and W504 in tox.ini

[abpbot]

A commit referencing this issue has landed:
​Issue 7196 - Ignore both flake8 W503 and W504 in tox.ini

[abpbot]

A commit referencing this issue has landed:
​Issue 7196 - Ignore both flake8 W503 and W504 in tox.ini