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

Support PEP 570 #867

Closed
sproshev opened this issue May 23, 2019 · 13 comments · Fixed by #918
Closed

Support PEP 570 #867

sproshev opened this issue May 23, 2019 · 13 comments · Fixed by #918

Comments

@sproshev
Copy link

sproshev commented May 23, 2019

https://www.python.org/dev/peps/pep-0570

E225 should be disabled for / in parameters list.

@asottile
Copy link
Member

for a concrete testcase:

def f(a, b, /, c, d, *, e, f):
    pass

currently failing with:

$ python t.py
$ pycodestyle t.py 
t.py:1:14: E225 missing whitespace around operator

@sandsbit
Copy link
Contributor

I think the issue can be closed now

@jirassimok
Copy link

jirassimok commented Feb 9, 2020

There's still one more case to cover: a function with only positional-only parameters still has the same problem, with a warning about a missing space before the closing parenthesis.

These are the additional test cases (for testsuite/python38.py) that should pass before this issue is closed:

#: Okay
def f(a, b, /):
    pass
#: E225:1:7
def f(/, a, b):
    pass

I have verified that you can't use the exact same fix for this as for other positional-only parameters.

@asottile
Copy link
Member

asottile commented Feb 9, 2020

@jirassimok can confirm, both of those shouldn't cause errors and currently do

curiously, my patch suggestion here seems to fix this: #872 (comment)

@jirassimok
Copy link

jirassimok commented Feb 9, 2020

I've been looking into it a bit. The problem occurs for two reasons.

First, the block starting at line 862 tries to ensure that there is equal whitespace on either side of the slash. Then in a future iteration of the loop, it finds the space missing and issues E225.

Clearing that requirement, as I did in a21fb57, does get rid of that problem, but it also prevents the same error if the slash is given as the first parameter, which is illegal.1

@asottile, your patch works because making the slash a unary operator lets it be handled by the block starting at line 886, which does basically the same thing as my commit (though it has another condition in it).

1 On second thought, I think that the last test case I added is actually incorrect. This is a style checker, not a syntax checker, so it's all right if it doesn't complain about a parameter list starting with a slash. This aligns with the behavior for cases like def f(*):.

@jirassimok
Copy link

jirassimok commented Feb 9, 2020

@asottile I agreed with the concerns others expressed in the discussion on #872, but I now believe your solution is better.

Ideally, *, **, and / would have some other special handling in parameter lists, but the code doesn't currently have a good way to do that, and your solution handles more cases (i.e. def f(a, /):, lambda x, /, y:, and lambda x, /:) than the current solution, which only works on one specific case.


The only changes I would make to your patch would be to eliminate the concerns others voiced, that / isn't actually a unary operator. I would add a comment above the "unary operators" listing what they are, and I'd probably change the variable's name.

# >> for Python 2, * and ** for unpacking, / for positional-only params
UNARY_LIKE_OP = ...  # [or something like UNARY_WHITESPACE_OP]

I think that would take care of FichteFoll's reasonable doubts about / not actually being a unary operator, and would help prevent anyone from misusing the variable in the future.

An additional comment where the variable is used would also be a good idea, and would match the current comments:

                # Allow positional-only parameters foo(a, /) or foo(a, /, b)

I've added the additional test cases (a function with just pos-only params and lambdas with pos-only params) in 9d5e2ac. I would open a pull request, but the code test cases fail, and I'd rather not spread these changes over multiple pull requests.

@aviramha
Copy link

This should be closed as #872 is merged, no?

@asottile
Copy link
Member

no, it is incomplete as per above: #867 (comment)

@asottile
Copy link
Member

I've created #918 to fix the "all positional arguments" case -- if someone would review it / try it out that would be greatly appreciated 🙏

@rharish101
Copy link

rharish101 commented Jul 14, 2020

Hi, W504 maybe is yet to support this. For example:

def really_long_name_for_a_function(
    very_long_argument_1, very_long_argument_2, /
):
    pass

It thinks that the slash is a binary operator. I can however silence it using a comma after the slash. I am using v2.6.0 on Arch Linux.

EDIT: Corrected syntax error

@asottile
Copy link
Member

@rharish101 the code you've written is a syntax error:

$ python3 t.py 
  File "t.py", line 2
    very_long_argument_1, very_long_argument_2, /f
                                                 ^
SyntaxError: invalid syntax

@rharish101
Copy link

Ah, I typed that wrong. There shouldn't be an f there. I've edited the original code.

@asottile
Copy link
Member

could you open a new issue with the exact command and version you're using -- this issue is rather old

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants