[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [patch #8527] Adding GRAPH command with simple scatterplot and histo

From: John Darrington
Subject: Re: [patch #8527] Adding GRAPH command with simple scatterplot and histogram (incl tests and doc)
Date: Wed, 1 Oct 2014 15:51:15 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

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[0]));
 ds_put_cstr (&title, " vs ");              
 ds_put_cstr (&title, var_to_string (cmd->dep_vars[1]));
 if (cmd->byvar)
     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 

if (cmd->byvar)
 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:  
   +++ b/src/language/stats/graph.c
   +  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,
                                   ds_cstr (&title),
                                   cmd->es[0].minimum, cmd->es[0].maximum,
                                   cmd->es[1].minimum, cmd->es[1].maximum);

  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 or any PGP keyserver for public key.

Attachment: signature.asc
Description: Digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]