Skip to content
This repository has been archived by the owner. It is now read-only.

Added generator send method #823

Merged
merged 15 commits into from Jun 3, 2018
Merged

Conversation

@BPYap
Copy link
Contributor

@BPYap BPYap commented May 25, 2018

This PR enables yield keyword to be used in expression and introduces send method to generator in voc.

Some example of things you can do with yield expression and send method:

  1. Echoes what is sent
def gen():
    # echoes what is sent in
    while True:
        x = print((yield)) 

g = gen()
next(g)
g.send("Hi there!")
g.send("Sup")

Output:

Hi there!
Sup
  1. Yield square of value sent
def gen():
    # yield square of value sent
    while True:
        x = yield("ready") 
        yield x ** 2

g = gen()
next(g)
print(g.send(2))
next(g)
print(g.send(-1))

Output:

4
1
  1. Comparing value sent into it
def gen():
    while True:
        x = 1 < (yield) < 5
        print(x)

g = gen()
next(g)
g.send(1)
g.send(3)

Output:

False
True

Note:

  • Once this PR and #821 are confirmed stable, I will implement the same features to yield from keyword
Copy link
Member

@freakboy3742 freakboy3742 left a comment

This is a good start. The test cases you've added are great.

I've left a couple of high level comments - mostly a need for clarification on exactly how you're handling some edge cases. I imagine I'll probably have some more comments once those high level questions have been addressed.

def unused():
yield from range(5)
print('Hello, world!')
print('Hello, world!')
Copy link
Member

@freakboy3742 freakboy3742 May 25, 2018

The (lack of) indentation on this line was intentional. The test needs some output to verify that something ran. The dummy "Hello world" is proof that the code ran. If it's in the unused() method, but unused() isn't invoked, then the print will never happen.

This entire test test_yield_from_not_used case will also become redundant as soon as yield from exists; the purpose of this test is to validate that just having yield from in your code won't cause a problem, as long as the yield from isn't actually on a live code path.

Copy link
Member

@freakboy3742 freakboy3742 Jun 1, 2018

To clarify; this change either needs to be reverted, with the patch from #821 deleting the test entirely.

def gen():
while True:
x = yield 1
yield x ** 2
Copy link
Member

@freakboy3742 freakboy3742 May 25, 2018

Is there any particular reason why a "power" operator is being used here? Is it just a mathematical operator to modify the value a bit? In terms of test naming, it seems like there is some sort of deeper significance to the choice of power specifically.

Copy link
Contributor Author

@BPYap BPYap Jun 1, 2018

I noticed pow is handled differently in visit_BinOp so I added this test case to see its behavior, it executes as intended and I have merged this test case under test_generator_yield_expr_binary_op

@@ -325,8 +325,79 @@ def visit_Delete(self, node):

@node_visitor
def visit_Assign(self, node):
# Evaluate the value
self.visit(node.value)
def push_message(node):
Copy link
Member

@freakboy3742 freakboy3742 May 25, 2018

This shouldn't be an closure method - it's not dependent on any of the context provided by visit_Assign. You can move it into a utility method at the level of the visitor (like e.g., full_classspec).

Copy link
Contributor Author

@BPYap BPYap May 25, 2018

Yes. Completely agree with that, I realized expression is more than just assignment and augmented assignment. I'm currently working on turning the block of code into an utility function which can be called in any node that evaluate an expression.


# walk subtree with node.value as root to find any yield node
nested_yield_node_found = False
for descendant_node in ast.walk(node.value):
Copy link
Member

@freakboy3742 freakboy3742 May 25, 2018

Is there a reason that this is handled as a separate walk, rather than just visiting node.value within the existing AST visitor that we're evaluating?

JavaOpcodes.INVOKEINTERFACE('org/python/Object', 'byValue', args=[], returns='Lorg/python/Object;')
)
elif hasattr(node, "lineno"): # a yield expression
# push NoneType object to stack
Copy link
Member

@freakboy3742 freakboy3742 May 25, 2018

It's not clear to me why a None is needed on the stack here - is something consuming it later?

(To that end, comments like "push None onto stack" aren't that helpful - it's fairly easy to see that the next line is "add none to stack"; the comment needs to say why that is the required course of action)

Copy link
Contributor Author

@BPYap BPYap Jun 1, 2018

This is to handle the statement yield which is equivalent to yield None . I have added a comment to it in latest PR

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Ok - this now has a merge conflict with #821; if you can resolve that conflict, this is ready to merge.

@BPYap
Copy link
Contributor Author

@BPYap BPYap commented Jun 3, 2018

@freakboy3742 Conflicts resolved as per request :-)

@freakboy3742 freakboy3742 merged commit 8be82ab into beeware:master Jun 3, 2018
5 checks passed
@BPYap BPYap deleted the generator_send_method branch Jun 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants