-
Notifications
You must be signed in to change notification settings - Fork 991
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 CHECK_RANGE to be i18n friendly #6530
Conversation
Generated via commit 7b40653 Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 3 minutes and 36 seconds Time taken to run |
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.
Sorry about the back and forth between what was originally in the code and a lot of extra strings (and extra work for the translators).
The buffer could be declared as |
Nice, done! CMIIW but this is preferable to a |
I think no problem at all! Most of the extra work is copy-pasting and they should all be one-after-the-other in the .po file. I view it as a big plus to translators who are empowered to make much better translations without having to awkwardly kludge their languages to fit what we provide. If anything the downside is on the code maintainers, but I think it's perfectly manageable. |
Another part of #6503.
I don't see a way to avoid introducing the new
columnDesc()
helper + associated "sub-buffer"; but I never liked that we're runningsnprintf()
with two "wasted" inputs whencolnum==0
previously anyway.An approach like
snprintf(..., strcat(FMT, colnum==0?_(" (target vector)"):_(" column %d named '%s')")), ...)
was attempted, but we get the-Wformat-security
issue because the formatting template is no longer a string literal -- it'd have to be"%s (%s)"
and then another sub-buffer approach IINM, roughly equivalent to what's done here.I also tried to only create the
column_desc
buffer insidecolumnDesc
but that would require returning a pointer to a local variable IINM: https://stackoverflow.com/q/12380758/3576984, hence anotherstatic char
array. I made it "large" to accommodate wide column names generously.This refactor also exposed the issue where for an
Rcomplex
value was being passed to%f
; for now I'm just usingval.r
and punting to follow-up to handle this more "properly": #6531.I think this is in the same spirit as @aitap's comment: #6503 (comment)