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.
> • 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'd also suggest using the logging methods to display info or debugging text on the terminal
>    
sounds good, example please
> • 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.
> • 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!
>
>