-
Notifications
You must be signed in to change notification settings - Fork 6
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
Post PyCon PEP cleanup #43
Conversation
# Conflicts: # docs/pep.rst
@lysnikolaou I worked with @jimbaker the last week to wrap up the PEP. It's in good shape for review. Jim's cleanup really helped readability. It tracks some changes in Do you mind giving it a look? |
Does anyone know if peps.python.org supports admonitions? (e.g. |
Thanks for the ping @pauleveritt! I'll take a look later today.
Yes, it does. There's a list of what the CPython documentation supports on this devguide page and I'm pretty sure that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a brief pass over the changes, which look good to me! Let's merge this, so that I can go through the PEP as a whole one more time and review it more deeply then.
def greet(*args): | ||
salutation, recipient, *_ = args | ||
_, getvalue = recipient | ||
return f"{salutation.title()} {getvalue().upper()}!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I missed an extra strip()
here. That, or removing the space. I might argue against the strip()
just to have one less thing for the user to think about, but either way:
return f"{salutation.title()} {getvalue().upper()}!" | |
return f"{salutation.title()}{getvalue().upper()}!" |
Or
return f"{salutation.title()} {getvalue().upper()}!" | |
return f"{salutation.title().strip()} {getvalue().upper()}!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do strip() in the other examples so I'll follow your first example. I don't believe any of the other examples have the extra-space problem.
Since this branch is deleted, I've started a new branch where I'll apply comments.
@lysnikolaou Think you have time to review more deeply before we announce to broader group? |
Sure! I'll have a look and post any comments I have later today. Not sure what the best place to do that would be though, I'll probably just open a new PR with my suggestions if I have any. |
@@ -194,7 +227,7 @@ For example: | |||
Valid Tag Names | |||
--------------- | |||
|
|||
The tag name can be any **undotted** name that isn't already an existing valid | |||
The tag name can be any *undotted* name that isn't already an existing valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, I'd love to understand the instinct here. I could imagine wanting, at minimum, dots:
import somelib
x = somelib.sometag"..."
Beyond dots, JavaScript allows tags to be arbitrary expressions. Perhaps that'd get too awkward in python?:
x = (lambda *args: args)"..."
Then again, PEP 614 allows for:
@(lambda *args: args)
def foo():
...
In any case, thank you... excited about this PEP!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will let @jimbaker address the motivation here. Glad you're excited about the PEP, lots we can do with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davepeck in principle we could allow both dotted names and arbitrary expressions, since both are currently invalid syntax. This was briefly mentioned here in the bikeshed issue, #3 (comment)
Given how much namespaces are used in current code - and the last admonition of import this
/Zen of Python, "Namespaces are one honking great idea -- let's do more of those!", perhaps we should relax this constraint; and possibly the use of parenthesized expression as well (to keep it reasonable, at least for human readers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I would also vote for a more relaxed syntax here. Dotted names at least, or even PEP 614-compatible syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Note that the syntax for PEP 614 is somewhat different because the @
introduces different opportunities for avoiding ambiguity, such as named expressions need not be parenthesized (https://peps.python.org/pep-0614/#named-expressions-need-not-be-parenthesized).
But atomic expressions should work (https://docs.python.org/3.9/reference/expressions.html#atoms)
Of course this does allow code like the following
(yield foo)"Hello, {name}"
where the tag function is provided by a value sent into the wrapping generator, but that's true of the generalization supported by PEP 614, and likely not not seen in actual decorator usage.
More focus on protocols, remove extraneous sections.