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

PEP 577: Revise & withdraw based on review feedback #665

Merged
merged 5 commits into from Jun 19, 2018

Conversation

Projects
None yet
4 participants
@ncoghlan
Copy link
Contributor

commented May 25, 2018

Guido providing some excellent feedback on the initial draft of PEP 577, and in
responding to it I realised that while the augmented assignment analogy had
helped me clarify what was bothering me about PEP 572's proposed handling
of previously undeclared assignment targets, actually allowing full inline
augmented assignments has enough other downsides that even I don't think
this particular PEP is a good way of resolving those concerns.

@ncoghlan ncoghlan changed the title PEP 577: Significant revisions based on review feedback WIP: PEP 577: Significant revisions based on review feedback May 25, 2018

@ncoghlan

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2018

Mostly working on responding to @gvanrossum's feedback in ncoghlan#1 (mainly the initial partial review at this stage - that gave me enough to think about that I haven't gone through the rest of the comments yet)

@gvanrossum
Copy link
Member

left a comment

A few nits.

statements do.
statements do. (The question of allowing expression level local variable
declarations at function scope is deliberately separated from the question of
allowing expression level name bindings, and deferred to a later PEP)

This comment has been minimized.

Copy link
@gvanrossum
PEP 572 quite reasonably notes that this results in ambiguity when assignment
expressions are used as function call arguments. This PEP resolves that concern
a different way by requiring that assignment expressions be parenthesised
when used as arguments to a function call (unless they're the sole argument).

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 25, 2018

Member

TBH I've been on the fence about doing the same thing in PEP 572.

and documentation of a reference implementation may show that it's actually
just as easy to propagate the explicit global variable declaration down into
the nested scope as needed, which would avoid the need to make this change to
the interaction between explicit ``nonlocal`` and ``global`` declarations.

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 25, 2018

Member

I strongly recommend doing that instead of allowing clashing explicit global and nonlocal declarations.

its absence irritating. Python has survived happily without expression level
name bindings *or* declarations for decades, so we can afford to give ourselves
a couple of years to decide if we really want *both* of those, or if expression
level bindings are sufficient.

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 25, 2018

Member

This is well reasoned.

This comment has been minimized.

Copy link
@tim-one

tim-one May 25, 2018

Member

Noting that every example in my PEP 572 Appendix would blow up (none of which involved a scoped expression, BTW). The majority of cases are simple and benefit from less redundancy and from removing a line of code. All of that is lost if I need to, e.g., add a semantically senseless diff = g = None before

if (diff := x - x_base) and (g := gcd(diff, n)) > 1:

or, possibly worse, give a type annotation I have no other use for (worse because then I also need to add import typing). Why is this code binding things to None when they're immediately re-bound? Or declaring their types in code that otherwise doesn't use types at all? Those questions can make code more mysterious.

I don't particularly mind if I need to add something to establish the intended scope for targets in scoped expressions, but for block-level code the intent of x := e in an expression is just plain obvious.

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan May 26, 2018

Author Contributor

Aye, I'd noticed that too. One intermediate option would be to only require the pre-declaration in cases where an explicit nested non-local reference would currently cause a syntax error. That said, if NAME: ... declared the scope of a name while still inferring its type from the first binding, then the required boilerplate would be relatively minimal.

I'll tinker with it a bit, since I still prefer having a real semantic reason for favouring the established assignment statements over assignment expressions in cases where the two collide.

@ncoghlan ncoghlan changed the title WIP: PEP 577: Significant revisions based on review feedback PEP 577: Revise & withdraw based on review feedback Jun 17, 2018

inline augmented assignments to be written using the ``operator`` module::

from operator import iadd
if (target := iadd(target, value)) < limit:

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum Jun 18, 2018

Member

If someone sent me a code review containing that in a PEP 572 world I'd reject it. In most cases it's clearer written as target := target + value -- and in those cases where it makes a difference (e.g. because you don't know if __iadd__ for this particular object returns self or a new object) you'd be better off using the non-inline syntax. So I don't see how this argument has any bearing on the debate about inline operators -- they don't add new functionality you can't write otherwise, they're meant to shorten code when it's straightforward, and if such a consideration was ever a concern the code is no longer straightforward.

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Jun 19, 2018

Author Contributor

This doesn't really have any bearing on PEP 572, except insofar as it supports the argument that syntactic support for inline augmented assignment isn't necessary.

and will instead be writing a replacement PEP that focuses specifically on the
handling of assignment targets which haven't already been declared as local
variables in the current scope (for both regular block scopes, and for scoped
expressions).

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum Jun 18, 2018

Member

Sigh. You keep changing your mind. That's fine, but it seems a burden to bother the PEP process with your personal thought processes to this extent. Can't you just present your next proposal on python-ideas first? Or if it benefits from trying to write about it carefully, write a blog post?

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Jun 18, 2018

Author Contributor

Sure, I can do that, but last time I asked Chris whether or not he was interested in a "given" based proposal, he said no (hence drafting a competing PEP in the first place, even though it morphed into something other than the original concept).

I'll still start with a python-ideas post, though - it may be that he'll find this variant of the idea more appealing than the one I asked him about previously.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

@ncoghlan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

I do hope you'll reconsider, because PEP 572's proposed scoping semantics are profoundly unintuitive for anyone that isn't already firmly locked into a Python 2-esque mental model of how comprehensions work.

(The new variant at ncoghlan#2 keeps the PEP 572 syntax and semantics except for cases that want to introduce a new name that isn't otherwise identified as being a local variable in the current scope. In those cases, and only in those cases, you have to tell both the reader and the compiler "Yes, I'm deliberately introducing a new name here").

@ncoghlan ncoghlan merged commit 8857369 into python:master Jun 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ncoghlan ncoghlan deleted the ncoghlan:pep-577-address-review-feedback branch Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.