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

Mon Sep 17 09:28:48 UTC 2012

Thanks Tobes.

On 17 September 2012 09:44, Toby Dacre <toby.okfn at gmail.com> wrote:

>
>
> 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
>>
>>
>
> _______________________________________________
> 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/d281260f/attachment-0001.html>