Pycodestyle: E251: triggers on line continuations

[jogo]

Unless I am missing something this should be allowed by E251. Since E251 is

" E251 unexpected spaces around keyword / parameter equals" and there are no extra spaces here

foo(bar=
    1) 

[sigmavirus24]

This is interesting. This changed in version 1.5.2. Prior to that, it was _not_ an error.

[jogo]

Looks like this was changed in 0a10f3ce45a25bbec3c64ec1eb46c13ed1386cf5

[jogo]

That patch is going beyond what pep8 requires and on the original bug report (#252) the author would rather see an extra long line then this, does pep8 allow for these extra long lines in this case?

I think 0a10f3c should be reverted as it goes beyond what pep8 requires.

[sigmavirus24]

It was added as a result of https://github.com/jcrocholl/pep8/issues/252 with which I'm not sure I disagree. An argument could be made that the newline character is a whitespace character covered under the rule. One could also argue (if you disagree about a newline character being whitespace) that the indentation before the value would be whitespace, but I'm not really interested in arguing.

@ncoghlan what do you think about this (given #256 does not include it)?

[sigmavirus24]

Personally, pep8 is flexible enough that I've always preferred simply changing the function/method call so that the parameters are all only indented once, e.g.,

parser.add_argument('--long-option',
                    default="/rather/long/filesystem/path/here/blah/blah/blah")

# Changes to

parser.add_argument(
    '--long-option',
    default="/rather/long/filesystem/path/here/blah/blah/blah"
)

[jogo]

What would you do in this case:

http://git.openstack.org/cgit/openstack/nova/tree/nova/tests/network/test_manager.py#n239

[jogo]

Code in question, so that when that file changes the example isn't lost.

            vif_dict = dict(bridge=network['bridge'],
                            cidr=net_v4['cidr'],
                            cidr_v6=net_v6['cidr'],
                            id=vif['id'],
                            multi_host=network.get_meta('multi_host', False),
                            injected=network.get_meta('injected', False),
                            bridge_interface=
                                network.get_meta('bridge_interface'),
                            vlan=network.get_meta('vlan'),
                            broadcast=str(net_v4.as_netaddr().broadcast),
                            dhcp_server=network.get_meta('dhcp_server',
                                net_v4['gateway']['address']),
                            dns=[ip['address'] for ip in net_v4['dns']],
                            gateway=net_v4['gateway']['address'],
                            gateway_v6=net_v6['gateway']['address'],
                            label=network['label'],
                            mac=vif['address'],
                            rxtx_cap=vif.get_meta('rxtx_cap'),
                            vif_type=vif['type'],
                            vif_devname=vif.get('devname'),
                            vif_uuid=vif['id'],
                            ovs_interfaceid=vif.get('ovs_interfaceid'),
                            qbh_params=vif.get('qbh_params'),
                            qbg_params=vif.get('qbg_params'),
                            should_create_vlan=
                                network.get_meta('should_create_vlan', False),
                            should_create_bridge=
                                network.get_meta('should_create_bridge',
                                                  False),
                            ip=net_v4['ips'][i]['address'],
                            ip_v6=net_v6['ips'][i]['address'],
                            netmask=str(net_v4.as_netaddr().netmask),
                            netmask_v6=net_v6.as_netaddr()._prefixlen,
                            physical_network=
                                network.get_meta('physical_network', None))

            self.assertThat(vif_dict, matchers.DictMatches(check))

[thezoggy]

@sigmavirus24 missing ' there

(edited by @florentx) thanks, I've added the missing quote

[florentx]

@jogo I hesitate to revert the patch for #252 .

If you are looking for alternative syntaxes, I could propose:

• switch to hanging indentation instead of visual indentation

            vif_dict = dict(
                bridge=network['bridge'],
                cidr=net_v4['cidr'],
                cidr_v6=net_v6['cidr'],
                id=vif['id'],
                multi_host=network.get_meta('multi_host', False),
                injected=network.get_meta('injected', False),
                bridge_interface=network.get_meta('bridge_interface'),
                #
            }

• or compute the long terms before building the dictionary

            multi_host = network.get_meta('multi_host', False)
            bridge_interface = network.get_meta('bridge_interface')

            vif_dict = dict(
                bridge=network['bridge'],
                cidr=net_v4['cidr'],
                cidr_v6=net_v6['cidr'],
                id=vif['id'],
                multi_host=multi_host,
                injected=network.get_meta('injected', False),
                bridge_interface=bridge_interface,
                #
            }

There's always a solution...

[sigmavirus24]

Thanks @thezoggy! I had just copied and pasted that from the original bug report and changed the indentation.

@jogo I'd echo @florentx's suggestions here. Both cases can cover 99% of the use cases I can imagine.

[jogo]

@florentx switching to hanging indentation instead of visual indentation would work, but I am still confused about the fix for #252, it doesn't sound like your fix fully addresses the request (missing support for extra long lines -- something I am sure folks won't universally accept).

While switching to hanging indent would work, I am concerned that this rule goes beyond what pep8 requires. Since this is "a tool to check your Python code against some of the style conventions in PEP 8." going beyond the pep8 scope in the default settings.

[sigmavirus24]

@jogo in that case this is starting to sound like part of #256.

[jogo]

@sigmavirus24 agreed

[sigmavirus24]

Here's a compromise on this issue: What if the behaviour (newlines around the assignment operator) becomes a new error code, e.g., E252. If that were to happen, either it could be off by default or OpenStack could ignore it more easily without ignoring the very valuable E251.

I am not entirely convinced that the result of #252 is a bug fix. It seems (obviously, in my opinion) more like an added feature and as such probably shouldn't have been released in 1.5.x but instead in 1.6.0. Fixing that would be fine in 1.5.x and fixing it in this manner would be fine as well. I'm also aware and appreciative of the hesitancy with which @florentx adds new error codes.

[jogo]

@sigmavirus24 that sounds like a great plan to me. I agree splitting the result of #252 out into its own check would be great, especially if it was off by default.

[sk-]

Any updates on this?

This is particularly annoying when using yapf, as it adds new lines after argument names to prevent lines to go over the limit. See https://github.com/google/yapf/issues/393.

[sigmavirus24]

@sk- none since 2014.

[luozhaoyu]

I encounter the same issue when using yapf 0.25.0 and pycodestyle 2.4.0 together

[MerdyNumber1]

any updates? same problem with yapf & flake8

[asottile]

commenting asking for updates is a waste of everyone's time, please don't do that @MerdyNumber1

if there were updates you would see either a comment here or a linked issue. in the future please use the voting feature instead of bumping threads needlessly