-
Notifications
You must be signed in to change notification settings - Fork 533
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
Refactor formatting #1335
Refactor formatting #1335
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1335 +/- ##
==========================================
- Coverage 91.79% 91.78% -0.01%
==========================================
Files 37 37
Lines 18175 18167 -8
==========================================
- Hits 16683 16674 -9
- Misses 1492 1493 +1 ☔ View full report in Codecov by Sentry. |
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.
Looks sane. Reviewing this is quite something, mostly because the huge indentation and lack of encapsulation of the old code. So this patch is surely a welcome improvement.
I know that the original format_inner
function used a simple w
as the main parameter name. By the time I arrived at the third patch I already forgot and had to go back. Maybe write
would be more verbose for people with a light memory like me.
Same goes for for Some(v)
. Still trying to figure out what it stands for... a padding value?
There were a few memory allocation changes from pass-by-reference to pass-by-copy (and back). I didn't track those through-full, but in the compiler I trust.
Thank you for reviewing!
Yes, this is just a copy-paste and minimal changes from by-reference to by-value as needed to keep the code compiling.
For context:
I hope this PR can land before it rots and needs to be redone from 0 to keep it a copy-paste. Hmmm, already has a conflict, I'll have to see about that. |
Rustfmt kept changing the formatting of this huge method. So part of the reason to split it over multiple methods is to avoid that. |
My own fault with #1473 😄. That's doable. |
ef6ee15
to
0466f74
Compare
@djc Do you want to give this a second look? |
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.
Looks great!
0466f74
to
9fb956e
Compare
We currently have a 234-line
format_inner
function.In #1163 I found a way to make it a bit nicer and faster.
This is only the initial refactoring, that splits it up into three methods on
DelayedFormat
.Every commit is as close to a copy-paste as compiles and passes rustfmt.