-
Notifications
You must be signed in to change notification settings - Fork 387
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
MatchOr and guard statements #158
base: main
Are you sure you want to change the base?
Conversation
…andling; 3. added more unit tests (with juliawgraham)
…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]>
Co-authored-by: Yuqi <[email protected]> Co-authored-by: Erica Fu <[email protected]> Co-authored-by: Lucybean-hi <[email protected]>
MatchOr and MatchAnd
…rds and matchor) Co-authored-by: Yuqi <[email protected]> Co-authored-by: Erica Fu <[email protected]> Co-authored-by: Lucybean-hi <[email protected]>
Thanks, could you follow the recent changes in main? |
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.
Only general comments
Could you clarify what those recent changes are? |
Your branch is behind the main branch. Can be resolved by merging main into this branch and resolving conflicts. |
Merging in new commits made one latexify main branch
Merging new main code into integration
We updated functions based on your suggestions, resolved all conflicts with the current main, and fixed all CI errors. Please take a check. |
r"\left\{ \begin{array}{ll} " | ||
+ r" \\ ".join(case_latexes) | ||
+ r" \end{array} \right." | ||
) | ||
|
||
latex_final = latex.replace("subject_name", subject_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.
Weird hack, might cause issues if there's an actual subject_name
in the
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.
Yes that is something we considered as well and would love to hear suggestions.
Since each of the visit_Match... functions cannot access the subject_latex variable(each of those functions can only have the ast node and pattern as the input), we had to find a workaround. We tried to use python format and % to insert the subject_latex but the use of % and {} in latex made this very difficult. Do you have any suggestions for how to better implement this (or possible a better name than "subject_name" to replace)
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.
Ah yeah I see the dilemma, bit perplexing lol. Not entirely sure how to solve but I'll give it some thought. Thanks for tackling match
statements btw! Really cool feature :)
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.
Not sure what @odashi thinks of this solution, but you could create a stack of _match_subject_stack: list[str] = []
, and at the very beginning of visit_Match
do something like
subject_latex = self._expression_codegen.visit(node.subject)
self._match_subject_stack.append(subject_latex)
and then right before you return do something like
latex = (
r"\left\{ \begin{array}{ll} "
+ r" \\ ".join(case_latexes)
+ r" \end{array} \right."
)
self._match_subject_stack.pop()
return latex
and you would also have
def visit_MatchValue(self, node: ast.MatchValue) -> str:
"""Visit a MatchValue node."""
latex = self._expression_codegen.visit(node.value)
return self._match_subject_stack[-1] + " = " + latex
A stack is only required if nested match statements is supported, which it seems like it is, although I have no idea if it's valid visit_MatchValue
, the LHS takes the appropriate _subject_latex
, i.e. the inner one should take the inner subject
while the outer one takes the outer.
edit: _subject_latexes
-> _match_subject_stack
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 think the @ZibingZhang 's solution is desirable at this point.
One minor point is that the private member _subject_latexes
sounds weird for every other visitors, more specific name (like _match_subject_stack
) would be better.
Co-authored-by: Zibing Zhang <[email protected]>
Co-authored-by: Zibing Zhang <[email protected]>
Co-authored-by: Zibing Zhang <[email protected]>
Co-authored-by: Zibing Zhang <[email protected]>
Co-authored-by: Zibing Zhang <[email protected]>
Co-authored-by: Zibing Zhang <[email protected]>
r"\left\{ \begin{array}{ll} " | ||
+ r" \\ ".join(case_latexes) | ||
+ r" \end{array} \right." | ||
) | ||
|
||
latex_final = latex.replace("subject_name", subject_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.
I think the @ZibingZhang 's solution is desirable at this point.
One minor point is that the private member _subject_latexes
sounds weird for every other visitors, more specific name (like _match_subject_stack
) would be better.
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 for your contribution. Since the new change introduced other issues, I will take over this branch to merge it ASAP.
@odashi if you're busy would you mind if i looked into this for merging? I think a lot of the conflicts were due to my changes |
@ZibingZhang Yeah please! |
MatchOr and MatchAnd and implemented and tested.
Overview
All of these produce the correct latex code when a user enters a MatchOr case or guard statements in a python match statement.
Details
made edits to visit_Match
created a new function visit_MatchOr
We worked with four collaborators
@erica-w-fu
@Lucybean-hi
@juliawgraham
@Yuqi07
References
#93
Blocked by
#146
#147