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

[Ian Murray]

Hello,

I've looked over the areas assigned to me:

 - lib/jinja_extensions.py
 - lib/render.py
 - lib/base.py
 - lib/helpers.py
 - lib/fanstatic*.py
 - templates

All looks fine to me:

 - I've pushed a few commits for some very minor things in helpers.py [1],
[2] and [3]
 - Templates look ok to me, but I'm probably not the best to judge.

 - Ian.

[1] Remove duplication in helpers.unselected_facet_items:
https://github.com/okfn/ckan/commit/c3d7a9ac5b8e3079db7e612ffb5c9066c53ca439
[2] Deprecate facet_title helper function:
https://github.com/okfn/ckan/commit/c2b604ea11fae8f783b33df0e628acfed377204b
 and
https://github.com/okfn/ckan/commit/07592dbdd0e075fda417ecd8a36b6dcaba989b12
[3] Cast integer-looking strings to ints, not floats:
https://github.com/okfn/ckan/commit/af2b29a386fb7e1b9c5f7be23d287c3964b9c49f

On 11 September 2012 16:03, Toby Dacre <toby.okfn at gmail.com> wrote:

>
>
> On 10 September 2012 16:26, Sean Hammond <sean.hammond at okfn.org> wrote:
>
>> On Sat, Sep 08, 2012 at 07:42:59PM +0200, Sean Hammond wrote:
>> > > > 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?
>> >
>> > Ah, working on a Saturday. I looked at this a little closer and it seems
>> > once I created a pot file and everything that these
>> > ckan/public/base/i18n/*.js files do actually contain translations to
>> > different languages, but only for a few strings, I guess only the ones
>> > that appear in javascript.
>> >
>> > So my guess is that Babel extracts strings from the javascript source
>> > into the po files, and this trans js command finds the translated
>> > strings in the po files and puts them in these i18n/*.js files for the
>> > js code to find them when it needs them at run time?
>> >
>> > I'll look at this some more later
>>
>> I've looked at this TranslationsCommand some more and it looks fine to
>> me, just a couple of minor points which I'll do if you agree:
>>
>> Sean,
>
> thanks for all your work on this
>
>
>> - I'd like to add a slightly longer docstring to say what `trans js`
>>   does
>>
>> yes I need to do this
>
>
>> - I guess we need to run trans js before each CKAN release and commit
>>   the results? This should be added to the Release Process wiki page
>>   (which is now on Github Wiki)
>>
>>
> yes that is the plan it's a release thing
>
>
>> - Is `trans mangle` really necessary? If you upload a pot file to
>>   Transifex, it can generate a po file for you with 100% strings
>>   translated into a fictional pseudo language where everything is really
>>   long strings of unicode characters. I found this worked well for
>>   coverage testing, and also tests handling of unicode and long strings
>>   all over the place.
>>
>> mangle was really to help with the development outside of transifex so I
> can remove it now that we have the translation worlking
>
>
>> I haven't looked at the javascript code itself and how it accesses these
>> translations, I'm probably not the best person to do that but let me
>> know if you want me to review it.
>>
>> I think that will be fine we are using jed which is some sort of
> javascript translation tool that aaron selected - I think this is a john
> martin issue if he feels there are problems.  Though I'm aware that we need
> to check the pluralize at some point
>
> cheers again
>
> toby
>
>> _______________________________________________
>> ckan-dev mailing list
>> ckan-dev at lists.okfn.org
>> http://lists.okfn.org/mailman/listinfo/ckan-dev
>>
>
>
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20120913/6448e5bc/attachment-0001.html>