Code review comment for lp://staging/~knitzsche/ubuntu-translations/ul10n-custom

Revision history for this message
David Planella (dpm) 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 :)
• Why add new files for each custom template type instead of simply adding new variables containing the templates in html_report_template.py?
• In ubuntu-translations-stats.py the --template-blacklist option is now missing a default
• 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?
• 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:

>>> 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)
• I'd also suggest using the logging methods to display info or debugging text on the terminal
• 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()
• 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.
• 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

Thanks!

« Back to merge proposal