[ckan-dev] 2375-demo-theme code review

Sat Sep 8 17:29:11 UTC 2012

> Translations: sean
> ------------------
> includes setup.py,

Do we still need:

    ('templates/**.txt', 'genshi', {
        'template_class': 'genshi.template:TextTemplate'
    }),

in message_extractors? Doesn't like there are any txt files in there
anymore. If not needed I'll delete this for you.

> lib/extract.py

I think it couldn't hurt for this module to have a docstring at the top
of the file explaining what the module is for. I know from setup.py that
it's there to provide the extract_ckan() string extractor function for
Babel, but I think looking at the file on its own it's not so obvious.

jinja2_cleaner looks like a helper function only meant to be used by
extract_ckan(), might make things clearer if it was called
_jinja2_cleaner(), just so it doesn't look like a public function the
module wants to export.

jinja_extensions seems to be a module-level variable that is only used
in one function, could be moved into the function, unless you think
there might be more functions that want it in future.

I'll make these changes if you agree.

Otherwise looks fine, it's a neat hack to detect genshi or jinja.

> lib/jinja_extensions.py trans tag changes CkanInternationalizationExtension

I only looked at CkanInternationalizationExtension, not the other jinja2
stuff.

I'm unsure about what's going on here. As I understand it, when we run
`python setup.py extract_messages` it's going to use the extract_ckan()
function from ckan/lib/extract.py to process the HTML files. For the
HTML files that are jinja2 templates, extract_ckan() will call
jinja2_cleaner() which will call regularise_html() on the strings.  So
the strings are regularised when they are extracted from the source
files.

But then we have the parse() method of CkanInternationalizationExtension
also calling regularise_html(). I don't get what's happening here. Why
do the strings need to be regularised twice?

My guess is that CkanInternationalizationExtension is used when the
strings are extracted from the templates at runtime, and they need to be
regularised at this time in order to match them against the regularised
strings in the mo files to find the translations to output?

Maybe CkanInternationalizationExtension needs a better docstring saying
what it does?

I'm not sure I'm super-convinced that this cleaning whitespace from the
strings is a good idea, I just don't like the thought that ckan messes
with the strings between templates and pot file, is it not better just
to expect the person writing the template, if they are using {% trans %}
tags, to understand that they have to not put extra whitespace and
newlines in there?

Feels a bit like we're second-guessing a decision already made by
jinja's i18n extension.

But if you're convinced that it's worthwhile to do it then fine with me.

By the way, regularise should be regularize if we are using American in
the source code? Let me know if you'd like me to change that.

> lib/cli.py TranslationsCommand

New `trans` paster command, the js command seems to make files for each
language in ckan/public/base/i18n/ like this:

    {
        "": {
            "domain": "ckan",
            "lang": "de",
            "plural-forms": "nplurals=2; plural=(n != 1)"
        }
    }

What are these files for? Maybe a better docstring than "generate the
javascript translations", the files that it generates don't contain any
translations. I guess this is part of the new system for handling
translations in javascript but I don't know anything about this system.
Maybe this is something I missed from the workshop in London. Maybe you
could give me a couple of pointers to get started understanding how this
works?

I also looked at the new `color` paster command, what does "less will
need to generate the css files after this has been run" mean? Is this
something that happens automatically or does the user have to do
something after runnung paster color? I feel this command needs better
documentation generally, it looks like the command lets me specify a
base color only, and then CKAN generates the rest of the colors based on
that? Once I've created a new color scheme, how do I use it?

> ckan/lib/formatters.py

Nice, looks like a good idea to me. Is all of the code using this e.g.
to format dates? Or do we still need to go through the code and switch
it all over to the new formatters?

> ckan/authz.py

This file isn't different from master -> 2375-demo-theme-development?