-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Ursula,
thank you for finally fixing this long-standing bug. This looks very
good although I do have a few comments. ;)

 review needs-fixing

Cheers,
Henning


Am 10.09.2009 19:00, Ursula Junque schrieb:
> Ursula Junque has proposed merging lp:~ursinha/launchpad/plural-forms-bug-156107 into lp:launchpad/devel.
> 
> Requested reviews:
>     Canonical Launchpad Engineering (launchpad)
> 
> = Summary =
> 
> This branch contains the fix for bug 156107, that talks about how plural forms are displayed in the language index page.
> 
> 
> == Implementation details ==
> This fix has two parts.
> 1) Migrating plural forms related methods to translations/utilities/pluralforms.py, newly created file.
> Some methods have been moved from utilities/gettext_po_parser.py to this one. Tests related to this
> change were updated as well.
> 
> 2) Writing a method that would create examples of plural forms, so we could display them friendly in
> the language index page, instead of the C formula as it was before.
> I've added a property to LanguageView, where the list of dicts that my function returns is slightly
> modified. Then the template just displays the list, as you can see in any language index page in
> this branch.
> 
> 
> == Tests ==
> bin/test -vvt language-views
> bin/test -vvt xx-language
> 
> 
> == Demo and Q/A ==
> To check the changed pages, go to:
>  - https://translations.launchpad.dev/+languages/sr
>  - https://translations.launchpad.dev/+languages/ru
>  - https://translations.launchpad.dev/+languages/it
> 
> It should display the new format in different cases.
> 
> 
> = Launchpad lint =
> 
> Checking for conflicts. and issues in doctests and templates.
> Running jslint, xmllint, pyflakes, and pylint.
> Using normal rules.
> 
> Linting changed files:
>  lib/lp/translations/utilities/doc/gettext_po_parser.txt
>  lib/lp/translations/scripts/fix_plural_forms.py
>  lib/lp/translations/utilities/pluralforms.py
>  lib/lp/translations/browser/tests/language-views.txt
>  lib/lp/translations/browser/language.py
>  lib/lp/translations/templates/language-index.pt
>  lib/lp/translations/stories/standalone/xx-language.txt
>  lib/lp/translations/utilities/gettext_po_parser.py
> 
> 

> === modified file 'lib/lp/translations/browser/language.py'
> --- lib/lp/translations/browser/language.py	2009-08-28 09:27:25 +0000
> +++ lib/lp/translations/browser/language.py	2009-09-10 13:31:25 +0000
> @@ -26,6 +26,7 @@
>      GetitemNavigation, LaunchpadView, LaunchpadFormView,
>      LaunchpadEditFormView, action, canonical_url, ContextMenu, NavigationMenu,
>      enabled_with_permission, Link, custom_widget)
> +from lp.translations.utilities.pluralforms import make_friendly_plural_forms
>  
>  from canonical.widgets import LabeledMultiCheckBoxWidget
>  
> @@ -156,6 +157,26 @@
>      def getTopContributors(self):
>          return self.context.translators[:20]
>  
> +    @property
> +    def friendly_plural_forms(self):
> +        """Formats the plural forms' example list.
> +
> +        It takes the list of examples for each plural form and transforms in a
> +        comma separated list to be displayed.
> +        """
> +        pluralforms_list = make_friendly_plural_forms(
> +                self.context.pluralexpression, self.context.pluralforms)
> +
> +        for item in pluralforms_list:
> +            examples = ", ".join(map(str, item['examples']))
> +            if len(item['examples']) != 1:
> +                examples += ", ..."
> +            else:
> +                examples += "."
> +            item['examples'] = examples
> +
> +        return pluralforms_list
> +
>  
>  class LanguageAdminView(LaunchpadEditFormView):
>      """Handle an admin form submission."""
> 
> === modified file 'lib/lp/translations/browser/tests/language-views.txt'
> --- lib/lp/translations/browser/tests/language-views.txt	2009-08-28 06:48:47 +0000
> +++ lib/lp/translations/browser/tests/language-views.txt	2009-09-10 14:04:22 +0000
> @@ -9,9 +9,9 @@
>      >>> from canonical.launchpad.layers import TranslationsLayer
>  
>      >>> language_set = getUtility(ILanguageSet)
> -    >>> language = language_set.getLanguageByCode('pt_BR')
> +    >>> portuguese = language_set.getLanguageByCode('pt_BR')
>      >>> language_admin_view = create_view(
> -    ...     language, '+admin', layer=TranslationsLayer)
> +    ...     portuguese, '+admin', layer=TranslationsLayer)
>      >>> language_admin_view.initialize()
>  
>  The language +admin view provides a label and a page_title for the page.
> @@ -73,3 +73,17 @@
>      >>> print language_add_view.next_url
>      http://translations.launchpad.dev/+languages/lp_US
>  
> +
> +View Language
> +------------------
> +
> +    >>> serbian = language_set.getLanguageByCode('sr')
> +    >>> language_view = create_initialized_view(serbian, '+index',
> +    ...     layer=TranslationsLayer)
> +
> +The friendlypluralforms function shows us a list of plural forms and a set of examples 
> +for each one, so one won't need to understand the plural formula expression to see how it works.
> +
> +    >>> language_view.friendly_plural_forms
> +    [{'examples': '1.', 'form': 0}, {'examples': '2, 3, 4, ...', 'form': 1}, {'examples': '5, 6, 7, 8, 9, 10, ...', 'form': 2}]

So, I have a problem with this although I have seen that it is used
elsewhere, too, even in this patch. Dictionary keys do not have a
defined order so it might be pure coincidence that they are in
alphabetical order here. But this could change with future versions of
Python and the test would break. Maybe you could ask around if that is
really OK or else come up with some code to check this.

> +
> 
> === modified file 'lib/lp/translations/scripts/fix_plural_forms.py'
[...]
> === modified file 'lib/lp/translations/stories/standalone/xx-language.txt'
> --- lib/lp/translations/stories/standalone/xx-language.txt	2009-08-28 18:35:37 +0000
> +++ lib/lp/translations/stories/standalone/xx-language.txt	2009-09-10 00:06:48 +0000
> @@ -92,11 +92,13 @@
>      >>> pluralforms_portlet = find_portlet(browser.contents, 'Plural forms')
>      >>> print pluralforms_portlet
>      <...
> -    There are 2 plural forms for Spanish.
> -    ...
> -    <pre>n != 1</pre>
> -    ...
> -
> +    Spanish has 2 plural forms:
> +    ...
> +    Form 0 for 1.
> +    ...
> +    Form 1 for 2, 3, 4, 5, 6, 7, ...
> +    ...
> +    

Do you know about extract_text? This would be a good place to use it.
lib/canonical/launchpad/doc/pagetest-helpers.txt

>      >>> translationteams_portlet = find_portlet(
>      ...     browser.contents, 'Translation teams')
>      >>> print translationteams_portlet
> 
> === modified file 'lib/lp/translations/templates/language-index.pt'
[...]
All good.
> === modified file 'lib/lp/translations/utilities/doc/gettext_po_parser.txt'
[...]
> === modified file 'lib/lp/translations/utilities/gettext_po_parser.py'
[...]
Moved code, very good!

> === added file 'lib/lp/translations/utilities/pluralforms.py'
> --- lib/lp/translations/utilities/pluralforms.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/utilities/pluralforms.py	2009-09-10 00:07:47 +0000
> @@ -0,0 +1,103 @@
> +# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +import re
> +import gettext
> +from lp.translations.interfaces.translations import (
> +    TranslationConstants)
> +
> +
> +class BadPluralExpression(Exception):
> +    """Local "escape hatch" exception for unusable plural expressions."""
> +
> +def make_friendly_plural_forms(expression, pluralforms_count):
> +    """Return a dict's list of plural forms' examples from an expression."""

Hm, I think this is not very clear, I am sorry. Could you describe the
returned value a bit more? "A list of dicts were each dict contains..."

Also, I do not see a test for this method. That would help to explain
it, too.

> +    expression_ = make_plural_function(expression)

You can safely use the same name on both sides of the = assignment
AFAIK. At least, I always do.

> +    forms = {}
> +
> +    for n in range(1, 200):

Hm, we very rarely use single letters as variable names. Never,
actually. The only reason here could be that 'n' is used in plural form
expressions, too, but I'd still like you to find a longer name.

> +        forms.setdefault(expression_(n), [])

I had to think and read the manual to understand what this is doing.
Please add a comment explaining it. Also, it would be a very good idea
to store the result of expression in a local variable.

  form = expression(n)
  # Create empty list if this form does not have one yet.
  forms.setdefault(form, [])

> +        # Giving a limit of 6 examples per plural form. If all the forms were
> +        # contemplated, it stops.
> +        if len(forms[expression_(n)]) == 6 and len(forms) == pluralforms_count:

Did you know we have a constant for this? :)
TranslationConstants.MAX_PLURAL_FORMS

> +            break
> +        if len(forms[expression_(n)]) == 6:
> +            continue
> +        forms[expression_(n)].append(n)
> +
> +    return [{'form':k, 'examples':v} for (k,v) in forms.iteritems()]

Single letters again ... How about form and examples, that's
straightforward.

> +
> +
[...]
Moved code went here. Thanks.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkqpR5gACgkQBT3oW1L17iihvQCdFSphlllH3x5v4WxXG0AT8oLm
mxIAniHi26+MqDdUTLvycvbIfeFIzGpj
=n51T
-----END PGP SIGNATURE-----