compiler: Simplify fission - #2973
Conversation
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 87.50000% with 5 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #2973 +/- ##
==========================================
+ Coverage 82.99% 83.57% +0.57%
==========================================
Files 257 257
Lines 53823 53841 +18
Branches 4609 4609
==========================================
+ Hits 44671 44997 +326
+ Misses 8339 8045 -294
+ Partials 813 799 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
EdCaunt
left a comment
There was a problem hiding this comment.
One minor comment but seems reasonable
Sorry, something went wrong.
| def scope(self): | ||
| return Scope(exprs=self.exprs) | ||
|
|
||
| @cached_property |
There was a problem hiding this comment.
Why cache this?
Sorry, something went wrong.
There was a problem hiding this comment.
that likely was for homogeneity I didn't even think about that, but yes, you're right.
If you don't mind, I'd drop it (along with the other useless ones in that class) in the subsequent PR
Sorry, something went wrong.
There was a problem hiding this comment.
Sure
Sorry, something went wrong.
without this simplification (which was done in 2021, now it seems unneeded, AFAICT based on the tests I've managed to run...) a new test in PRO would fail
I don't see the logic behind it, so unless CI/CD complains, I'd drop it
It might also be a leftover from some other tweaks that was later undone... not sure!