El dc 06 de 10 de 2010 a les 16:37 +0000, en/na Kyle Nitzsche va
escriure:
> On 10/06/2010 12:01 PM, David Planella wrote:
> > Thanks for the great work on this Kyle. My comments:
> >
> > • I'd strongly suggest that the README file stays. I can assure I'll need it when in two weeks time I no longer remember what the script does :)
> >    
> this was an error. add it back please.
> > • Why add new files for each custom template type instead of simply adding new variables containing the templates in html_report_template.py?
> >    
> to keep ubuntu stuff and custom stuff separated. and to keep these files 
> shortish and easier to edit.
> > • In ubuntu-translations-stats.py the --template-blacklist option is now missing a default
> >    
> that's intentional and relates by the new support for release-specific 
> blacklist. that is, the default is determined by the config/settings 
> distro_codename field. you only use this option if you want to use a 
> different file that you specify.
> > • In ubuntu-translations-stats.py, do we need the --no-file option at all? Can we just not specify a blacklist if we don't want to use one?
> >    
> as per the above comment, by default the blacklist for the release in 
> settings.conf is used. I am agnostic as to the best way to specify using 
> no 'file' blacklist.

I think it is probably redundant. We've already got -b to specify the
blacklist file. If we don't want one, we simply don't specify it neither
on the command line (-b) nor in the config file. So my suggestion would
be to remove it and go for fewer options and saner defaults.

> > • In the get_src_pkgs_from_binary_pkgs() you could consider using the apt_pkg Python library instead of calling an external process. You could try something like:
> >    
> I'll take a look at this point further - seems right.
> >    
> >>>> pkgs = map(lambda a: a.strip(), open("bla").readlines())
> >>>> import apt_pkg
> >>>> apt_pkg.init()
> >>>> sources = apt_pkg.GetPkgSrcRecords()
> >>>> sources.Restart()
> >>>> for pkg in pkgs:
> >>>>          
> > ...     while sources.Lookup(pkg):
> > ...             print sources.Package
> >
> > (Credit to dholbach for showing me this some time ago)
> >
> > • I'd suggest using the format() method instead of things such as 'print "text (" + variable + ")"' - e.g. "text ({1})".format(variable)
> >    
> why? it is less intuitive to read (and code) with format.

I guess it's a matter of style or personal preference. I personally find
it easier to read
'Coordinates: {0}, {1}'.format(latitude, longitude) than
'Coordinates: ' + latitude + ', ' + longitude

In the first form the text is one single unbroken string (where {0} and
{1} could also be replaced by variable names), and it has also the
benefit to provide a translator-friendly string (i.e. not split) in case
it ever needed to be made translatable. Not that it applies in this
script, I just find it good practice.

> > • I'd also suggest using the logging methods to display info or debugging text on the terminal
> >    
> sounds good, example please

You can see it in the trunk branch, all files have got logging
statements. E.g.

  logger.info('Getting language list...')

Documentation: http://docs.python.org/library/logging.html

> > • In ul10n_stats/ul10n_stats_calc.py variations of the block below are executed 4 times. I'd suggest writing a function containing that code, where the type of report can be passed as an argument. Rather than having separate python modules containing different reports, it might be wort exploring having them all in one module and provide a function in that file to select them.
> > 		f_ = codecs.open(os.path.join('reports', 'custom-excluded.html'), 'w', 'utf-8')
> > 		f_.write(excluded.ubuntu_template % variables)
> > 		f_.close()
> >    
> yes, i was going to do that, just didn't get to it.
> > • When merging, please take into account the changes in trunk that should stay: mainly the html + css cleanup and adaptation of the table to the Light theme, the logging mechanism, the new classes for language operations, and the bugfix for the language ordering in the report.
> >    
> I am not sure how to do that. you said push a branch and make it a merge 
> proposal, which is what I did. open to suggestions for how we can both 
> actively develop the same branch.

I'm not saying anything different. You've made a merge proposal with the
intention to merge to trunk. The merge has got conflicts, which I assume
you'll resolve and then you'll merge the branch into trunk. I was just
mentioning the most visible changes in trunk since your branch diverged,
to make sure they are not lost when resolving the conflicts.

> > • As the original script is growing to be a full-fledged project, I think we should look into adopting some minimal coding conventions. I would suggest https://dev.launchpad.net/PythonStyleGuide
> >
> >    
> good idea
> > Thanks!
> >
> >    
> 


-- 
David Planella
Ubuntu Translations Coordinator
www.ubuntu.com / www.davidplanella.wordpress.com
www.identi.ca/dplanella / www.twitter.com/dplanella