Optimize DupN code generation & fix testing bug
#623
Conversation
|
I believe the fact that 538f784 passes tests is a bug in our test code. I'm investigating further |
jasonpaulos
changed the title
DupN code generationDupN code generation & fix testing bug
| options | ||
| ) | ||
| return dupn_srt, dupn_end | ||
| return DupN(self.auto_instance, self.count - 1).__teal__(options) |
| @@ -1,4 +1,5 @@ | |||
| from pathlib import Path | |||
| from difflib import unified_diff | |||
tests/compile_asserts.py
Outdated
| current_quote: str | None = None | ||
| for i, char in enumerate(line): | ||
| for qt in quote_types: | ||
| if char == qt: |
There was a problem hiding this comment.
Actually this probably needs to take into account escaped quotes, e.g. \"
There was a problem hiding this comment.
I'm not sure the purpose of processing each line, versus just keeping the raw original. IMO, for this test code it's okay (even preferrable?) to fail because the generated teal is non-identical, even if it's semantically equivalent. I think we would want to know when this happens, and in that case we could decide to simply replace the old fixtures with the new ones.
There was a problem hiding this comment.
Ok, I remember now. When I originally wrote this test, I had some hand-commented code which I didn't want to fail the diff. I think at this point, the usage has grown much larger than I originally anticipated. So the comments I just wrote, convince me that it's better to not try and pre-process. I.e., don't ignore comments. If we agree that this approach is reasonable, I'm more than happy to replace the original fixtures with fixtures which will pass the tests.
There was a problem hiding this comment.
I agree that it's preferable to fail if the generated code is different (otherwise we shouldn't be checking in any code for this).
Some checked in code is augmented with comments, so a direct line by line comparison will fail, even though the ops are the same. This processing function is responsible for stripping away the comments plus any difference in whitespace.
I haven't looked in detail at all the code, but my impression is we'd want to keep the additional user-generated comments if at all possible.
There was a problem hiding this comment.
Ah, looks like we replied at the same time. If you're ok with getting rid of the comments, I'm happy to make that change to simplify this comparison code
There was a problem hiding this comment.
Yeah for sure. Those original old fixtures don't need their hand crafted comments anymore.
There was a problem hiding this comment.
Updated in 27f530e
There was a problem hiding this comment.
I agree with the approach to replace dupn with dup or nothing whenever possible. I also support bringing in the more standard united_diff() into compile_asserts.py vs. the previous ad-hoc diffing function.
I left a question regarding processing of the lines from Teal file fixtures, vs. just reading them raw.
Optimize code generated by the
DupNexpression.Prior to this PR, that expression always used the
dupnop. However, if only 1 additional value is needed, thedupop can be used to save a byte. And if no additional values are needed and you still useDupN, no additional op is needed.While testing this change, I noticed a bug in the
assert_teal_as_expectedfunction fromtests/compile_asserts.py. This function used to only check that the expected line of code prefixes the actual line, and unfortunatelydupprefixesdupn 1. I decided to upgrade the comparison function by using thedifflibpackageand processing each line to strip away all auxiliary information.