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 Yield from statement #821

Merged
merged 6 commits into from Jun 3, 2018

Conversation

Projects
None yet
2 participants
@BPYap
Contributor

BPYap commented May 23, 2018

This commit only added support for yield from x or yield from foo() by itself (see samples in test),
the assignment statement x = yield from foo() is currently not supported as generator.send() is currently WIP

@BPYap BPYap changed the title from Yield from to Added support for Yield from statement May 23, 2018

@BPYap BPYap referenced this pull request May 25, 2018

Merged

Added generator send method #823

@freakboy3742

This looks like a good first pass, and it's clearly got the basics working - good work!

However, this code doesn't cover all the error conditions that PEP380 covers. PEP380 includes branches for GeneratorExit, StopIteration including a value, and so on.

This might be something that you'll be able to tackle once you've got send() and the other generator methods implemented - is that correct?

In cases like this where there is known "missing" functionality, it can help to either (a) include comments in the code that flag where "extra" logic will come later (e.g., # TODO: handle GeneratorError); and/or add tests for the edge cases you know are problematic (and then mark them as @expectedFailure, possibly also with a comment). That way, you can signal to a reviewer that you've thought of the edge cases, you just haven't addressed them all yet.

@expectedFailure
def test_yield_from_used(self):

This comment has been minimized.

@freakboy3742

freakboy3742 May 25, 2018

Member

This is one of those rare situations where a test can be removed. The test itself is a much simpler version of what you've tested thoroughly with your other yield from tests.

print(i)
""")
def test_chainning_yieldfrom(self):

This comment has been minimized.

@freakboy3742

freakboy3742 May 25, 2018

Member

spelling: chaining

)
self.context.add_opcodes(
TRY(),
)

This comment has been minimized.

@freakboy3742

freakboy3742 May 25, 2018

Member

As a style thing, it's preferable to include as many opcodes as possible in a single add_opcodes call. The end result is exactly the same in terms of functionality, but there's less code to read, which can make the intent of the code easier to interpret for those coming in later.

@@ -1712,7 +1712,8 @@ def visit_GeneratorExp(self, node):
@node_visitor
def visit_Yield(self, node):
self.visit(node.value)
if hasattr(node, "lineno"):
self.visit(node.value)

This comment has been minimized.

@freakboy3742

freakboy3742 May 25, 2018

Member

It isn't clear to me why lineno is a relevant attribute here. Is there a type of AST node that doesn't have a linen that might be visited?

At the very least, a comment explaining the need for this branch is required; better still would be a test case that exercises this case (assuming there isn't one already)

This comment has been minimized.

@BPYap

BPYap May 25, 2018

Contributor

This is sort of a 'hackish' way to differentiate a programmatically defined yield node from those generated normally by ast.parse.

Combined with #823, there will be 3 types of yield node passed into visit_Yield method:

  1. Ordinary yield node generated from a line of code (eg yield x). In this case we need to visit x (the default implementation).
  2. Ordinary yield node generated from a line of code, but without value/operand (eg yield or x=yield). In this case we push a org.python.types.NoneType.NONE object on stack without visiting the none value.
  3. Programmatically defined yield node (in line 1778 self.visit(ast.Yield(None))) This node is created in visit_YieldFrom and the absent of lineno is use to indicate that a value has been pushed on stack hence there is no need to call self.visit(node.value)

Edit: I just remembered it is possible to add custom attribute to a class so I'm thinking of using a flag (i.e skip_visit) as custom attribute of the programmatically defined yield node instead of the lineno to increase clarity, what do you think?

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 1, 2018

Member

Ok - that reasoning makes sense; however, it isn't immediately obvious from the code. Adding a comment explaining what is going on would be helpful. A custom attribute isn't necessary; as long as the code explains what it's doing, that will be sufficient.

@freakboy3742

Looks great!

@freakboy3742 freakboy3742 merged commit f5564fc into pybee:master Jun 3, 2018

5 checks passed

beekeeper:0/beefore:javacheckstyle Java lint checks passed.
Details
beekeeper:0/beefore:pycodestyle Python lint checks passed.
Details
beekeeper:1/smoke-test Smoke build (Python 3.4) passed.
Details
beekeeper:2/full-test:py3.5 Python 3.5 tests passed.
Details
beekeeper:2/full-test:py3.6 Python 3.6 tests passed.
Details

@BPYap BPYap deleted the BPYap:yield_from branch Jun 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment