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

Implement yield-from in Python 2.x as a macro #686

Closed
wants to merge 1 commit into from

Conversation

@paultag
Copy link
Member

commented Nov 15, 2014

And who said you can't teach an old dog new tricks.

@paultag paultag force-pushed the paultag:paultag/feature/yield-from branch from d7a2088 to a1ecdd0 Nov 15, 2014

@paultag

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2014

@paultag

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2014

Hum. On a plane - anyone object to eol'ing py 3.2?

@berkerpeksag

This comment has been minimized.

Copy link
Member

commented Nov 15, 2014

anyone object to eol'ing py 3.2?

+100 (and also 2.6)

@Foxboron

This comment has been minimized.

Copy link
Member

commented Nov 15, 2014

How about we just scrap 2.x while we are on it?

On 11/15/2014 03:17 PM, Berker Peksag wrote:

anyone object to eol'ing py 3.2?

+100 (and also 2.6)


Reply to this email directly or view it on GitHub #686 (comment).

@rcarmo

This comment has been minimized.

Copy link

commented Nov 15, 2014

Please don't scrap 2.x - IronPython and PyPy still havent caught up, and you wouldn't believe where I'm running Hy these days...

@Foxboron

This comment has been minimized.

Copy link
Member

commented Nov 15, 2014

@rcarmo It's more of an internal joke then reality. Don't worry :3

Implement yield-from in Python 2.x as a macro
  And who said you can't teach an old dog new tricks.

  ... but at the same time, drop Python 3.2 for not knowing this new
  trick.

@paultag paultag force-pushed the paultag:paultag/feature/yield-from branch from a1ecdd0 to 84c5aaa Nov 15, 2014

@paultag

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2014

3.2 dropped from the testing tools in this commit; a full drop of 3.2 will come with a compiler and compat cleanup

@paultag

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2014

2.6 is on probation until it breaks like 3.2 broke for us here -- 2.7 is going to stick around for a while, but slowly grow more hacks like this :)

@theanalyst

This comment has been minimized.

Copy link
Member

commented Nov 15, 2014

👍 with dropping 3.2. (please don't kill 2.7 yet )

@paultag

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2014

Cool. Updated to kill 3.2 in the tests. Good for review!

@paultag paultag referenced this pull request Nov 15, 2014
@cwebber

This comment has been minimized.

Copy link
Member

commented Nov 17, 2014

It looks fine to me, though it is incomplete IIRC. If exceptions are called, they should be able to be propagated via the coroutine's .throw() method, correct? I don't think this happens here?

https://www.python.org/dev/peps/pep-0380/#formal-semantics gives a sense of how this works I think.

We could merge this as-is, but it would need to be documented that this functionality is not included.

@cwebber

This comment has been minimized.

Copy link
Member

commented Nov 21, 2014

@paultag is right and I am wrong. Exception handling in the yield-from macro works as expected. As such I merged @paultag's branch. I also added a test to prove that paultag is right, so we know for eternity that exception handling is indeed working right. (I also fixed some indentation on paultag's test while I was at it!)

Nice work @paultag! I'm glad I was wrong, and impressed by the conciseness of the implementation!

@cwebber cwebber closed this Nov 21, 2014

@paultag

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2014

@cwebber ❤️

@@ -179,6 +179,25 @@
~@body))))


(if-python2

This comment has been minimized.

Copy link
@superbobry

superbobry Nov 21, 2014

Hello, I read about this PR in the dustycloud blogpost.

The idea of using macros to emulate yield from sounds wonderful. However, the implementation doesn't handle some edge cases described in the Formal Semantics section of PEP-380. Was this deliberate or is it just a proof-of-concept at the moment?

This comment has been minimized.

Copy link
@cwebber

cwebber Nov 21, 2014

Member

Which edge cases does it not handle? I believed also that it was failling to handle exception propagation, but it turned out that this was not true (we now have a test that proves as such). If there are edge cases not being handled, please submit test cases and file bugs! Thanks! :)

This comment has been minimized.

Copy link
@pyos

pyos Nov 21, 2014

Contributor

@superbobry makes a good point. Here's a concrete example of where the semantics differ:

hy 0.10.1 using CPython(default) 2.7.8 on Linux
=> (defn f []
...   (yield 1)
...   (try
...     (yield 2)
...     (catch [e ValueError] (yield 3))))
=> (defn g []
...   (yield-from (f))
...   (assert true))  ; see issue #691
=> (setv x (g))
=> (next x)
1L
=> (next x)
2L
=> (x.throw ValueError)
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "<input>", line 257, in g  ; note that ValueError didn't even reach `f`
ValueError

as opposed to

hy 0.10.1 using CPython(default) 3.4.2 on Linux
...
=> (next x)
2
=> (x.throw ValueError)
3

This would pose a problem, for example, if one was to reimplement asyncio on top of yield-from, as it uses task.throw(CancelledError) to abort running tasks. Obviously, if this is not fixed, only the outermost task would be aborted; any tasks it may be waiting on will completely avoid being cancelled.

This comment has been minimized.

Copy link
@cwebber

cwebber Nov 21, 2014

Member

Good point! File a bug! :)

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