[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [patch #8527] Adding GRAPH command with simple scatterplot and histo
Re: [patch #8527] Adding GRAPH command with simple scatterplot and histogram (incl tests and doc)
Wed, 1 Oct 2014 15:51:15 +0200
I finally got around to reviewing this patch.
In a word: Ausgezeichnet!
0. There is a potential internationalisation issue in the function
ds_init_cstr (&title, var_to_string (cmd->dep_vars));
ds_put_cstr (&title, " vs ");
ds_put_cstr (&title, var_to_string (cmd->dep_vars));
ds_put_cstr (&title, " by ");
ds_put_cstr (&title, var_to_string (cmd->byvar));
This is not been marked for translation (and cannot be anyway, because it has
been constructed on-the-fly)
You should instead use something similiar to
ds_put_format (&title, _("%s vs. %s by %s"), var_to_string (xxx),
var_to_string (yyy), var_to_string (zzz));
ds_put_format (&title, _("%s vs. %s"), var_to_string (xxx), var_to_string
That way, the translators get complete strings to translate instead of single
(Without context, how would you translate the word "by" into German? bei,
durch, indem, neben ....?)
Also, note that in other places we have used "vs." (with a period) as the
abbreviation for the latin "versus".
Other than that, I can find only minor cosmetic issues:
1. Copyright date:
+ Copyright (C) 2012, 2013 Free Software Foundation, Inc.
It's now 2014 right! - if this file was derived from another existing file,
it is correct that the existing dates should be kept. But the current year
should be appended.
+ // xrchart_line (cr, geom, npp->slope, npp->intercept,
+ // npp->y_first, npp->y_last, XRCHART_DIM_Y);
We don't use // style comments (because older compilers don't support them),
and it is best not to comment
out code anyway - just remove it if its not needed.
3. In a few places: there is missing whitespace: Eg:
scatterplot = scatterplot_create(input,
In PSPP we try to follow the GNU Coding Standards, which recommend a space
before each (
4. Although it hurts my imperialistic pride,
msg (MW, _("Maximum number of scatterplot categories reached."
"Your BY variable has too many distinct values."
"The colouring of the plot will not be correct"));
we decided some years ago to use US instead of commonwealth English. That
"coloring" instead of "colouring".`
Thanks for the patch. A number of people have been asking for this, for some
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285 A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.
Description: Digital signature
|[Prev in Thread]
||[Next in Thread]|
- Re: [patch #8527] Adding GRAPH command with simple scatterplot and histogram (incl tests and doc),
John Darrington <=