-
Notifications
You must be signed in to change notification settings - Fork 19
refactor: Rename optimize() -> try_optimize_tx_size() #617
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
base: snawaz/commit-diff-buffer
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
c3bd6e4 to
b72b7e3
Compare
b72b7e3 to
bc42634
Compare
aa4b132 to
1d13c07
Compare
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 don't mind the change to highlight what we exactly optimizing, but few points:
- This should be opened into
master, as it does not necessarily refer tocommit-diff-bufferbranch. Also this waycommit-diff-bufferbranch will be less noisy in diff withmaster - I'd prefer name
optimize_tx_size, since in theory we iteratively optimizing it and not doing it right away, at least in context ofTaskStrategist
Yes, that's a great name as well. We can keep that instead. Or maybe
Yes. It should have been onto And no, it wont be noisy. This PR wont merge to |
taco-paco
left a comment
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 suggestion to rename to try_optimize_* makes the most sense.

The current name
optimize()is too generic and doesn’t convey what exactly to optimize for: space or performance?.Initially, I assumed it was meant for performance optimization, but after reading through the relevant code, especially
optimize_strategy(), I realized the optimization is actually about reducing transaction size.Changes:
optimize()→try_optimize_tx_size()optimize_strategy()→try_optimize_tx_size_if_needed()I think this makes the purpose explicit and avoids ambiguity. The function focuses on minimizing transaction size (not any size in general), not improving speed or compute efficiency.