Factor out expression codegen from function codegen #155
Conversation
| @@ -0,0 +1,704 @@ | |||
| """Codegen for single expressions.""" | |||
There was a problem hiding this comment.
How about splitting other objects than ExpressionCodegen into other files too in this PR? Maybe definitions in constants.py should also be integrated as well.
There was a problem hiding this comment.
sure yeah! maybe a bulitin_functions.py? this would essentially just turn into a rename of the file?
There was a problem hiding this comment.
On second thought maybe save other splitting for another PR?
This PR is getting very big, and trying to solve merge conflicts is mildly painful. Would like to merge this in ASAP and further refactors can come in subsequent PRs.
There was a problem hiding this comment.
It's fine to prepare a separate PR for this refactoring.
There was a problem hiding this comment.
bulitin_functions.py looks confusing. I guess simply rules.py or something else would be good.
src/latexify/analyzers_test.py
Outdated
| "3 / 4", | ||
| ], | ||
| ) | ||
| def test_reduce_stop_parameter_not_binop_with_op_add_or_sub(code: str) -> None: |
There was a problem hiding this comment.
- I think we don't need to separate test cases into 2 parts.
- Shrink this king of long test names, which are not convenient to read.
There was a problem hiding this comment.
Okay, going to bring test case into one method, which solves the issue of long name
| @@ -0,0 +1,704 @@ | |||
| """Codegen for single expressions.""" | |||
There was a problem hiding this comment.
It's fine to prepare a separate PR for this refactoring.
| @@ -0,0 +1,704 @@ | |||
| """Codegen for single expressions.""" | |||
There was a problem hiding this comment.
bulitin_functions.py looks confusing. I guess simply rules.py or something else would be good.
|
@ZibingZhang @odashi |
|
Thanks for the catch, apologies for that mistake I made when resolving merge conflicts. |
|
Great, thanks both! (this kind of collision often happens when we applied large changes, so please never mind. but we should fix it ASAP) |
This PR does not add / remove any functionality, nor does it add / remove any tests. It's purely a refactor.
Overview
Split
function_codegeninto two files like so:function_codegenthat implements visiting module and statements nodes, and delegates toexpression_codegenfor expression nodes.expression_codegenthat implements visiting expression nodes (as well asast.comprehension.Considerations
How should we test
function_codegencomprehensively? All tests thatexpression_codegenhave will apply tofunction_codegen, at least for the time being.References
Initial idea: #57 (comment)
Factor this change out of algorithmic codegen PR: #152 (comment)
Blocked by
None