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

Mon Sep 17 08:44:20 UTC 2012

On 13 September 2012 14:06, Ian Murray <ian.murray at okfn.org> wrote:

> 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
>
> Ian,

Thanks, I've cherry-picked these into master.  I think the last one is
unnecessary due to the number ending up cast as a float but overall it
probably makes the code more readable/extendable so I've pulled it too.

Cheers
Toby

>
> 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
>>
>>
>
> _______________________________________________
> 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/20120917/1ed0ff64/attachment-0001.html>