-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix fmt UTF-8 ─
character as fill
#18533
Conversation
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.
nice work! please add a test though to be sure and so it doesn't regress :)
Co-authored-by: Jacob Young <jacobly0@users.noreply.github.com>
Test added. Thanks for feedback! 3 pipelines are green on my fork https://github.com/vinnichase/zig/actions/runs/7510739735 |
eb11b31
to
d279fe9
Compare
OK gotcha! I misunderstood your first suggestion.
Added your suggested Do we need a test for |
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Jacob Young <jacobly0@users.noreply.github.com>
Co-authored-by: Jacob Young <jacobly0@users.noreply.github.com>
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.
Implementation LGTM
std.debug.print("wow thank you so much for the babysitting! {s:💙<3}\n", .{ "" }); I very much appreciate that. learned some good lessons today. |
Co-authored-by: Jacob Young <jacobly0@users.noreply.github.com>
fixes #17025
I created this PR as a zig learning opportunity.
My changes allowed me to exchange
fmt.zig
in my project, compile and run it on two platforms with the fill character being correctly formatted.Of course my changes involve replacing
u8
s withu21
in various positions which allocates more memory. I clearly don't have the foresight to know if that is against the goals forfmt
or causes other problems. I wasn't even sure if I should report a bug until I found the existing one since It is a very narrow edge case only covering one character. Maybe it was even intended to keep it ASCII letters.However as of now I find it more intuitive to allow all unicode characters.
I'm also happy to discuss implementation details and revise my solution many times 😄