Merge lp://staging/~knitzsche/ubuntu-translations/ul10n-custom into lp://staging/~ubuntu-translations-coordinators/ubuntu-translations/ul10n-stats

with rev 33 and 34 I added release specific black lists. Now config/ has blacklist.lucid.conf and blacklist.maverick.conf. Default is derived from config/settings distro_codename field. -b argument still allows you to use specified file.

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!

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 gr...

So I/we need to work out the conflicts.

Since we don't yet have a working style for that, I am continuing on with development. (I will do whatever is necessary to make this right!)

With the latest commits to my merge proposal branch, I fix a bug and add the following: now all files generated in reports/ start with <basename>, where <basename> is returned by utils.get_basename() function and where it is derived from defaults + config/settings.conf + arguments. see bzr log

I should perhaps add that the point of this is that I'd like to be able to run the whole shebang multiple times and have all generated files (and links in html to them) be named according to that execution's target, so that one targeted execution does not overwrite the files of a differently targeted execution.

ok, forget those two last comments (for the moment), divergence issue prevented push success. I've gotta get this branch proliferation under control. ;)

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...')

Document...

some of the items David and I discussed as potential changes have not yet been done. These can wait until we get back to a common merged trunk.

I noticed there is a bug (unicode error) in trunk before this merge that causes crash when not obtaining languages from a local file with the -l <file> option.

I handled the merge imperfectly and so my bzr log comments after 24 (there were about 10 more) in my branch are lost. Chock that up to experience. Among the key points are:
 * by default (with no args) file blacklisting is enabled AND there are release-specific blacklist files in config/, for example: config/blacklist.lucid.conf. To disable file blacklisting, use the -f argument.
 * -n <name> causes all generated files (html and data files) in reports/ to start with that as basename. in general,
  > no args: "<distro_id>-<release>" (for example: ubuntu-10.10")
  > -c (for custom) and -n <name>: "<name>" (for example: "thisproject">
  > -c (and no -n): "custom-<distro_id>-<release>" (for example: "custom-ubuntu-10.04")

 * -m <file> starts from manifest (and requires -c)
 * -d <file> starts from file of gettext domains, one per line
 * reports/ includes generated files for a) conversion to html as needed, b) debugging/data analysis, where all files start we basename, as described above