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.

Sign up for GitHub

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

Jump to bottom

refactor: prefix trimming as a node transformer #113

Closed

Eric-Mendes wants to merge 12 commits into google:main from Eric-Mendes:refactor/prefix-trimming-as-node-transformer

Eric-Mendes commented Nov 16, 2022 •

edited

Loading

Overview

Implemented the change proposed in issue #81.

Details

Couldn't figure out a nice way of implementing automatic detection of used libraries and its aliases as discussed in #88, so in this implementation the user needs to pass them in a set[str] called prefixes.

References

Blocked by

NA


          refactor: prefix trimming as a node transformer

7a4dce1

Eric-Mendes requested a review from odashi as a code owner

November 16, 2022 00:41


          removed old prefix trimming from FunctionCodegen

833a44b

odashi reviewed

View reviewed changes

src/latexify/transformers/prefix_trimmer.py Outdated Show resolved Hide resolved

src/latexify/frontend.py Outdated Show resolved Hide resolved

src/latexify/frontend.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer_test.py Outdated Show resolved Hide resolved

Eric-Mendes and others added 6 commits

November 15, 2022 22:25


          Update src/latexify/transformers/prefix_trimmer.py

b80546b

Co-authored-by: Yusuke Oda <yusuke.oda@predicate.jp>


          Update src/latexify/transformers/prefix_trimmer.py

325d076

Co-authored-by: Yusuke Oda <yusuke.oda@predicate.jp>


          Update src/latexify/frontend.py

e869f95

Co-authored-by: Yusuke Oda <yusuke.oda@predicate.jp>


          Update src/latexify/transformers/prefix_trimmer.py

daa800c

Co-authored-by: Yusuke Oda <yusuke.oda@predicate.jp>


          Update src/latexify/transformers/prefix_trimmer.py

2735abb

Co-authored-by: Yusuke Oda <yusuke.oda@predicate.jp>


          Update src/latexify/transformers/prefix_trimmer.py

a6ab9d0

Co-authored-by: Yusuke Oda <yusuke.oda@predicate.jp>

odashi added the feature label

odashi added this to the v0.3 milestone

odashi added the refactoring label


          added more tests & refactored code

442761d

Collaborator

odashi commented Nov 16, 2022

@Eric-Mendes If you finished refactoring, please push the re-request button (the arrow circle) on the right pane.

Eric-Mendes requested a review from odashi

November 16, 2022 05:05

odashi reviewed

View reviewed changes

src/latexify/frontend.py Outdated Show resolved Hide resolved

src/latexify/frontend.py Outdated Show resolved Hide resolved

src/latexify/frontend.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer.py

+                      """
+                      self._prefixes = prefixes
+                  def visit_Call(self, node: ast.Call) -> ast.Call:

Collaborator

odashi Nov 16, 2022

I think visit_Call is no longer necessary, as you implemented visit_Attribute instead, which can process every Attribute node in the tree.

Author

Eric-Mendes Nov 16, 2022

Without it tests fail. Not sure why

Collaborator

odashi Nov 17, 2022

Could you provide the example outputs you observed?

src/latexify/transformers/prefix_trimmer.py

+                      while isinstance(node_value, ast.Attribute):
+                          node_value = node_value.value
+                      # if outermost prefix matches any given prefix, loose

Collaborator

odashi Nov 16, 2022 •

edited

Loading

I think this policy is not safe as the dropped subtree may involve the stuff that the users didn't expect. It would be better to drop only the leftmost prefix, and provide a way to specify longer chains to drop: "foo.bar" (but this may be somewhat beyond the scope of this pull request).

src/latexify/transformers/prefix_trimmer_test.py Outdated Show resolved Hide resolved

src/latexify/transformers/prefix_trimmer_test.py Outdated Show resolved Hide resolved

Collaborator

odashi commented Nov 16, 2022 •

edited

Loading

@Eric-Mendes Hi, it looks all failed tests belong to function_codegen_test.py. These tests only invoke FunctionCodegen without any preprocessing.

Since this pull request removed the trimming behavior from FunctionCodegen, the codegen no longer recognize any prefixes, resulting in some test failures. You need to fix the input text of test cases to have trimmed identifiers, e.g., math.prod -> prod

Eric-Mendes and others added 3 commits

November 16, 2022 20:06


          resolved some conversations

207d991


          removed prefixes from function_codegen_test

c4f1129


          Merge branch 'main' into refactor/prefix-trimming-as-node-transformer

a3facaf

Collaborator

odashi commented Nov 23, 2022

@Eric-Mendes Hi, it looks the test is still failing, and there's some conflicts against main. I'd appreciate it if you could resolve these issues.

Collaborator

odashi commented Nov 27, 2022

Closes this pull request due to more than 1 week inactivity.

odashi closed this

ZibingZhang mentioned this pull request

Prefix trimming as a node transformer #137

Closed

odashi mentioned this pull request

Prefix trimmer #138

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature refactoring