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

Added support for PEP 570 (#867) #872

Merged
merged 5 commits into from
Jan 30, 2020
Merged

Conversation

sandsbit
Copy link
Contributor

@sandsbit sandsbit commented Jul 1, 2019

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code seems fine, can you make testsuite/python38.py with:

def f(a, /, b):
    pass

@asottile
Copy link
Member

fwiw, this patch also fixes the problem, I don't know which one is more correct (there other "unary operators" covering the splat / etc. operations in this list (it's how named-only arguments aren't flagged):

diff --git a/pycodestyle.py b/pycodestyle.py
index 6ad5456..a0c2898 100755
--- a/pycodestyle.py
+++ b/pycodestyle.py
@@ -112,7 +112,7 @@ REPORT_FORMAT = {
 PyCF_ONLY_AST = 1024
 SINGLETONS = frozenset(['False', 'None', 'True'])
 KEYWORDS = frozenset(keyword.kwlist + ['print', 'async']) - SINGLETONS
-UNARY_OPERATORS = frozenset(['>>', '**', '*', '+', '-'])
+UNARY_OPERATORS = frozenset(['>>', '**', '*', '/', '+', '-'])
 ARITHMETIC_OP = frozenset(['**', '*', '/', '//', '+', '-'])
 WS_OPTIONAL_OPERATORS = ARITHMETIC_OP.union(['^', '&', '|', '<<', '>>', '%'])
 # Warn for -> function annotation operator in py3.5+ (issue 803)
diff --git a/testsuite/python38.py b/testsuite/python38.py
new file mode 100644
index 0000000..2132cd5
--- /dev/null
+++ b/testsuite/python38.py
@@ -0,0 +1,2 @@
+def f(a, /, b):
+    pass

@mikeholler
Copy link

Any updates on this? I'm having issues as well.

@aviramha
Copy link

code seems fine, can you make testsuite/python38.py with:

def f(a, /, b):
    pass

Seems author is stale, should I/anyone else just create a new PR with a test added?

@sandsbit
Copy link
Contributor Author

I will add tests tonight

@aviramha
Copy link

Any update?

@@ -0,0 +1,3 @@
#: Okay
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be python38.py

pycodestyle.py Outdated Show resolved Hide resolved
sandsbit and others added 2 commits January 22, 2020 20:21
Co-Authored-By: Anthony Sottile <asottile@umich.edu>
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems fine to me, a smaller patch however:

-UNARY_OPERATORS = frozenset(['>>', '**', '*', '+', '-'])
+UNARY_OPERATORS = frozenset(['>>', '**', '*', '/', '+', '-'])

(this is how keyword-only arguments are handled currently)

@asottile
Copy link
Member

actually, looks like you'll need to rebase now since python38.py got created on master

@sandsbit
Copy link
Contributor Author

sandsbit commented Jan 22, 2020

this seems fine to me, a smaller patch however:

-UNARY_OPERATORS = frozenset(['>>', '**', '*', '+', '-'])
+UNARY_OPERATORS = frozenset(['>>', '**', '*', '/', '+', '-'])

I think it is not a good idea 'cause in this case '/' is not an unary operator

@asottile
Copy link
Member

this seems fine to me, a smaller patch however:

-UNARY_OPERATORS = frozenset(['>>', '**', '*', '+', '-'])
+UNARY_OPERATORS = frozenset(['>>', '**', '*', '/', '+', '-'])

(this is how keyword-only arguments are handled currently)
I think it is not a good idea 'cause in this case '/' is not an unary operator

neither is >> nor * nor **

@FichteFoll
Copy link
Contributor

* and ** kinda are unary for unpacking, but they aren't operators in that case. I have no idea why >> is in that list, however.

Either way, since * and ** are allowed for unpacking in quite a few situations, it kind of makes sense to have them in that list, but / is really only valid in the parameter list and the suggested patch is more targeted and thus makes more sense to me.

@asottile
Copy link
Member

>> is probably for python 2

@jakabk
Copy link

jakabk commented Jan 29, 2020

Waiting for release

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 this pull request may close these issues.

7 participants