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

Add support for ast.IfExp #111

Merged
merged 5 commits into from
Nov 16, 2022
Merged

Add support for ast.IfExp #111

merged 5 commits into from
Nov 16, 2022

Conversation

kshxtij
Copy link
Contributor

@kshxtij kshxtij commented Nov 15, 2022

Overview

Add support for a if x else b (ast.IfExp) statements.

Details

Screenshot 2022-11-15 at 13 54 16

References

Blocked by

NA

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kshxtij kshxtij requested a review from odashi as a code owner November 15, 2022 13:59
@google-cla
Copy link

google-cla bot commented Nov 15, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this is really good! Could you prepare some unit tests in function_codegen_test.py as well?

latex = r"\left\{ \begin{array}{ll} "
cond_latex = self.visit(node.test)
true_latex = self.visit(node.body)
false_latex = self.visit(node.orelse)
Copy link
Collaborator

@odashi odashi Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I can make some follow-up pull requests for this comment.

I think this line does not process the recursion. e.g.:

if a:
    return x
elif b:
    return y
else:
    return z

and

return x if a else (y if b else z)

should express the same logic, but this impl may generate a different result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2022-11-15 at 21 24 42

I was able to process the recursion you highlighted, but wanted to know if we need to support recursion within both code blocks of the if then else statement like shown in the 2nd example.

Copy link
Collaborator

@odashi odashi Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In visit_If the current implementation processes only the orelse subtree. I think we could choose the same behavior at this point.

@odashi odashi added this to the v0.3 milestone Nov 15, 2022
@odashi
Copy link
Collaborator

odashi commented Nov 16, 2022

Great work, thanks!

@odashi
Copy link
Collaborator

odashi commented Nov 16, 2022

Btw, CIs (Black and flake8) are still failing, I will merge this PR after resolving these issues.

@odashi odashi added the feature label Nov 16, 2022
@kshxtij kshxtij requested a review from odashi November 16, 2022 22:52
@odashi odashi merged commit 142bc32 into google:main Nov 16, 2022
@odashi odashi mentioned this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants