-
Notifications
You must be signed in to change notification settings - Fork 25
Add dict comprehension Support #217
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
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
hi @Zehen-249 ! thanks for working on that. ensure you are running the pre-commit locally, and ping me here when all the tests on ci pass, please. |
f04be0d
to
ca55364
Compare
This pull request has been marked as stale because it has been |
@Zehen-249 , maybe the issue was just locally .. on ci it worked. |
@@ -1,5 +1,10 @@ | |||
"""Test Python Transpiler.""" | |||
|
|||
# for running tests from local astx module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove these comments.
btw, if you install with poetry install
, you shouldn't need any of these hacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay sure I will do it. I actually did used poetry.
|
||
def __init__( | ||
self, | ||
key: Identifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key and value probably should be Expr instead of Indentifier ... but maybe we need to discuss a bit other part of this class.
let's consider this example:
>>> ast.dump(ast.parse("{'y': x*x for x in my_list}"))
"Module(body=[Expr(value=DictComp(key=Constant(value='y'), value=BinOp(left=Name(id='x', ctx=Load()), op=Mult(), right=Name(id='x', ctx=Load())), generators=[comprehension(target=Name(id='x', ctx=Store()), iter=Name(id='my_list', ctx=Load()), ifs=[], is_async=0)]))], type_ignores=[])"
se it seems we will also need to add ifs
and the comprehension class as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will work on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did considered Expr for these variables but during pre-commit checks I was failing some checks bcoz, Expr did not have value attribute. But my tests was doing fine coz I had initialized key value as identifiers in the object creation.
Could you pls remove this stale |
@Zehen-249 , I just added support for the Comprehension class, please update your PR to use that: #225 |
hey @xmnlab I have created a new PR for this issue. Should CLOSE this PR. |
Pull Request description
Add DictComprehension class to support Dictionary comprehension
Solve #198
proposed in this PR, in both image and ASCII formats. For more
information, check this Google Colab notebook:
https://colab.research.google.com/drive/1xXwHmOMkJKFSmhRvn4WYfSAsdDzMnawf?usp=sharing
This PR is a:
About this PR:
Author's checklist:
complexity.
Additional information
screenshot that helps to show the changes proposed
Copy and paste this template for your review's note: