-
Notifications
You must be signed in to change notification settings - Fork 392
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
Match, match_case, and MatchValue #146
Conversation
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. |
@Casper-Guo What is 15-313? If it is not related to the code itself, could you discuss it outside this repository? |
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.
Thanks! Overall the policy looks fine, but there are some essential points to fix. Please take a look.
latex += r"\end{array} \right." | ||
return latex | ||
|
||
def visit_match_case(self, node: ast.match_case) -> str: |
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.
Wrong type: this function returns tuple[str, str]
. If you need to implement a method that returns other types than str
, please implement non-visitor function (functions that don't have visit_
prefix).
I also think this visitor is not necessary. Since the cases
member is always match_case
and Match
is the only syntax where match_case
is used, it could be implemented directly in visit_Match
.
""" | ||
match x: | ||
case 0: | ||
return 1 |
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.
So this (and the following test case) should raise the error, according to the comment above.
We have updated the code according to your comments. More specifically, on this PR, we merged visit_match_case into visit_Match. On PR #147 , we added error handling for wildcards. More specifically, wildcards could only appear, and must appear, as the last match case. We also updated our unit tests to reflect these changes. |
"""Visit a match node""" | ||
latex = r"\left\{ \begin{array}{ll} " | ||
subject_latex = self.visit(node.subject) | ||
for i, match_case in enumerate(node.cases): |
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.
the enumerator i
is never used.
def visit_MatchValue(self, node: ast.MatchValue) -> str: | ||
"""Visit a MatchValue node""" | ||
latex = self.visit(node.value) | ||
return r" = " + latex |
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.
return r" = " + latex | |
return " = " + latex |
Could you merge the main to resolve conflicts as well? |
…in Match node; 3. added corresponding unit tests Co-authored-by: Lucybean-hi <[email protected]> Co-authored-by: juliawgraham <[email protected]> Co-authored-by: Erica Fu <[email protected]>
We have updated the code according to your comments and resolved the merge conflicts shown. |
def _reduce_stop_parameter(self, node: ast.BinOp) -> str: | ||
# ast.Constant class is added in Python 3.8 | ||
# ast.Num is the relevant node type in previous versions | ||
if sys.version_info.minor < 8: | ||
if isinstance(node.right, ast.Num): | ||
if isinstance(node.op, ast.Add): | ||
if node.right.n == 1: | ||
upper = "{" + self.visit(node.left) + "}" | ||
else: | ||
reduced_constant = ast.Num(node.right.n - 1) | ||
new_node = ast.BinOp(node.left, node.op, reduced_constant) | ||
upper = "{" + self.visit(new_node) + "}" | ||
else: | ||
if node.right.n == -1: | ||
upper = "{" + self.visit(node.left) + "}" | ||
else: | ||
reduced_constant = ast.Num(node.right.n + 1) | ||
new_node = ast.BinOp(node.left, node.op, reduced_constant) | ||
upper = "{" + self.visit(new_node) + "}" | ||
else: | ||
upper = "{" + self.visit(node) + "}" | ||
else: | ||
if isinstance(node.right, ast.Constant): | ||
if isinstance(node.op, ast.Add): | ||
if node.right.value == 1: | ||
upper = "{" + self.visit(node.left) + "}" | ||
else: | ||
reduced_constant = ast.Constant(node.right.value - 1) | ||
new_node = ast.BinOp(node.left, node.op, reduced_constant) | ||
upper = "{" + self.visit(new_node) + "}" | ||
else: | ||
if node.right.value == -1: | ||
upper = "{" + self.visit(node.left) + "}" | ||
else: | ||
reduced_constant = ast.Constant(node.right.value + 1) | ||
new_node = ast.BinOp(node.left, node.op, reduced_constant) | ||
upper = "{" + self.visit(new_node) + "}" | ||
else: | ||
upper = "{" + self.visit(node) + "}" | ||
|
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.
Remove this function.
And resolve all CI errors. |
Co-authored-by: Yuqi <[email protected]> Co-authored-by: Erica Fu <[email protected]>
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.
Note: This pull request involves errors, we need to merge #147 ASAP after this PR.
Will fix test errors by myself. |
Match, match_case, and MatchValue are implemented and tested.
Overview
All of these produce the correct latex code when a user enters match python code with cases that are only matching the values.
Details
Added three new functions to function_codegen.py.
visit_Match provides the overall structure for the case statements in latex and loops through different match cases
visit_match_case separates the condition and return value and calls whichever type of match is being called
visit_MatchValue compares the value of the condition
Added two tests to function_codegen_test.py
test_matchvalue(), tests if one MatchValue case will return the correct latex code
test_multiple_matchvalue(), tests if multiple MatchValue cases will return the correct latex code
We worked with four collaborators
@erica-w-fu
@Lucybean-hi
@juliawgraham
@Yuqi07
References
#93