Launchpad itself

Merge lp:~ursinha/launchpad/plural-forms-bug-156107 into lp:launchpad

  1. plural-forms-bug-156107
  2. Merge into devel
Proposed by Ursula Junque
Status: Merged
Merged at revision: not available
Proposed branch: lp:~ursinha/launchpad/plural-forms-bug-156107
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~ursinha/launchpad/plural-forms-bug-156107
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Approve
Ursula Junque (community) Needs Resubmitting
Review via email: mp+11542@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ursula Junque (ursinha) wrote :

= 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

Revision history for this message
Henning Eggers (henninge) wrote :
Download full text (9.4 KiB)

-----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.
> + """
>...

Read more...

review: Needs Fixing
Revision history for this message
Ursula Junque (ursinha) wrote :
Download full text (11.3 KiB)

[snip]

> > === 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.

I've workarounded this one by checking what's inside the dicts, is it necessary to show the structure dict/list or the content is fine?

[snip]

> > === 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

Done. Thanks for the tip on the extract_text method. :)

>
> > >>> translationteams_portlet = find_portlet(
> > ... browser.contents, 'Translation teams')

[snip]

> > === added file 'lib/lp/translations/utilities...

review: Needs Resubmitting
Revision history for this message
Henning Eggers (henninge) wrote :

As I have seen your new incremental patch, you resolved all the issues we talked about. Wonderful!

Thanks for your great effort! ;-)

Henning

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/translations/browser/language.py'
2--- lib/lp/translations/browser/language.py 2009-08-28 09:27:25 +0000
3+++ lib/lp/translations/browser/language.py 2009-09-10 13:31:25 +0000
4@@ -26,6 +26,7 @@
5 GetitemNavigation, LaunchpadView, LaunchpadFormView,
6 LaunchpadEditFormView, action, canonical_url, ContextMenu, NavigationMenu,
7 enabled_with_permission, Link, custom_widget)
8+from lp.translations.utilities.pluralforms import make_friendly_plural_forms
9
10 from canonical.widgets import LabeledMultiCheckBoxWidget
11
12@@ -156,6 +157,26 @@
13 def getTopContributors(self):
14 return self.context.translators[:20]
15
16+ @property
17+ def friendly_plural_forms(self):
18+ """Formats the plural forms' example list.
19+
20+ It takes the list of examples for each plural form and transforms in a
21+ comma separated list to be displayed.
22+ """
23+ pluralforms_list = make_friendly_plural_forms(
24+ self.context.pluralexpression, self.context.pluralforms)
25+
26+ for item in pluralforms_list:
27+ examples = ", ".join(map(str, item['examples']))
28+ if len(item['examples']) != 1:
29+ examples += ", ..."
30+ else:
31+ examples += "."
32+ item['examples'] = examples
33+
34+ return pluralforms_list
35+
36
37 class LanguageAdminView(LaunchpadEditFormView):
38 """Handle an admin form submission."""
39
40=== modified file 'lib/lp/translations/browser/tests/language-views.txt'
41--- lib/lp/translations/browser/tests/language-views.txt 2009-08-28 06:48:47 +0000
42+++ lib/lp/translations/browser/tests/language-views.txt 2009-09-10 14:04:22 +0000
43@@ -9,9 +9,9 @@
44 >>> from canonical.launchpad.layers import TranslationsLayer
45
46 >>> language_set = getUtility(ILanguageSet)
47- >>> language = language_set.getLanguageByCode('pt_BR')
48+ >>> portuguese = language_set.getLanguageByCode('pt_BR')
49 >>> language_admin_view = create_view(
50- ... language, '+admin', layer=TranslationsLayer)
51+ ... portuguese, '+admin', layer=TranslationsLayer)
52 >>> language_admin_view.initialize()
53
54 The language +admin view provides a label and a page_title for the page.
55@@ -73,3 +73,17 @@
56 >>> print language_add_view.next_url
57 http://translations.launchpad.dev/+languages/lp_US
58
59+
60+View Language
61+------------------
62+
63+ >>> serbian = language_set.getLanguageByCode('sr')
64+ >>> language_view = create_initialized_view(serbian, '+index',
65+ ... layer=TranslationsLayer)
66+
67+The friendlypluralforms function shows us a list of plural forms and a set of examples
68+for each one, so one won't need to understand the plural formula expression to see how it works.
69+
70+ >>> language_view.friendly_plural_forms
71+ [{'examples': '1.', 'form': 0}, {'examples': '2, 3, 4, ...', 'form': 1}, {'examples': '5, 6, 7, 8, 9, 10, ...', 'form': 2}]
72+
73
74=== modified file 'lib/lp/translations/scripts/fix_plural_forms.py'
75--- lib/lp/translations/scripts/fix_plural_forms.py 2009-07-17 00:26:05 +0000
76+++ lib/lp/translations/scripts/fix_plural_forms.py 2009-09-08 17:18:04 +0000
77@@ -16,7 +16,8 @@
78 from lp.translations.model.translationmessage import TranslationMessage
79 from lp.translations.interfaces.translations import TranslationConstants
80 from lp.translations.utilities.gettext_po_parser import (
81- POHeader, plural_form_mapper)
82+ POHeader)
83+from lp.translations.utilities.pluralforms import plural_form_mapper
84
85
86 def get_mapping_for_pofile_plurals(pofile):
87
88=== modified file 'lib/lp/translations/stories/standalone/xx-language.txt'
89--- lib/lp/translations/stories/standalone/xx-language.txt 2009-08-28 18:35:37 +0000
90+++ lib/lp/translations/stories/standalone/xx-language.txt 2009-09-10 00:06:48 +0000
91@@ -92,11 +92,13 @@
92 >>> pluralforms_portlet = find_portlet(browser.contents, 'Plural forms')
93 >>> print pluralforms_portlet
94 <...
95- There are 2 plural forms for Spanish.
96- ...
97- <pre>n != 1</pre>
98- ...
99-
100+ Spanish has 2 plural forms:
101+ ...
102+ Form 0 for 1.
103+ ...
104+ Form 1 for 2, 3, 4, 5, 6, 7, ...
105+ ...
106+
107 >>> translationteams_portlet = find_portlet(
108 ... browser.contents, 'Translation teams')
109 >>> print translationteams_portlet
110
111=== modified file 'lib/lp/translations/templates/language-index.pt'
112--- lib/lp/translations/templates/language-index.pt 2009-08-28 18:36:27 +0000
113+++ lib/lp/translations/templates/language-index.pt 2009-09-10 14:03:15 +0000
114@@ -32,15 +32,15 @@
115 <h2>Plural forms</h2>
116 <tal:has_pluralforms condition="context/pluralexpression">
117 <p>
118- There are <tal:pluralforms replace="context/pluralforms">2</tal:pluralforms>
119- plural forms for <tal:language replace="view/language_name">Espa&ntilde;ol</tal:language>.
120- </p>
121- <p>
122- The default plural form expression for <tal:language replace="view/language_name">Espa&ntilde;ol</tal:language> is:
123- </p>
124- <pre tal:content="context/pluralexpression">
125- n != 1
126- </pre>
127+ <tal:language replace="view/language_name">Espa&ntilde;ol</tal:language> has
128+ <tal:pluralforms replace="context/pluralforms">2</tal:pluralforms> plural forms:
129+ </p>
130+ <div tal:repeat="pluralform view/friendly_plural_forms">
131+ <p>
132+ Form <tal:index replace="pluralform/form">0</tal:index> for
133+ <tal:examples replace="pluralform/examples">1, 2, 3...</tal:examples>
134+ </p>
135+ </div>
136 <p>
137 When translating into this language, you need to be able to
138 express plural forms effectively. The plural form expression tells
139@@ -114,7 +114,6 @@
140 </p>
141 </div>
142 </div>
143-
144 <div class="yui-u">
145 <div class="portlet">
146 <h2>Countries</h2>
147
148=== modified file 'lib/lp/translations/utilities/doc/gettext_po_parser.txt'
149--- lib/lp/translations/utilities/doc/gettext_po_parser.txt 2009-07-02 17:16:50 +0000
150+++ lib/lp/translations/utilities/doc/gettext_po_parser.txt 2009-09-08 17:18:55 +0000
151@@ -435,7 +435,7 @@
152
153 >>> form_1 = 'n!=1'
154 >>> form_2 = 'n==1'
155- >>> from lp.translations.utilities.gettext_po_parser import (
156+ >>> from lp.translations.utilities.pluralforms import (
157 ... plural_form_mapper)
158 >>> plural_form_mapper(form_1, form_2)
159 {0: 1, 1: 0, 2: 2, 3: 3, 4: 4, 5: 5}
160
161=== modified file 'lib/lp/translations/utilities/gettext_po_parser.py'
162--- lib/lp/translations/utilities/gettext_po_parser.py 2009-07-30 17:56:41 +0000
163+++ lib/lp/translations/utilities/gettext_po_parser.py 2009-09-08 17:17:07 +0000
164@@ -13,7 +13,6 @@
165 __all__ = [
166 'POHeader',
167 'POParser',
168- 'plural_form_mapper',
169 ]
170
171 import gettext
172@@ -38,84 +37,9 @@
173 TranslationFileData,
174 TranslationMessageData)
175 from canonical.launchpad.versioninfo import revno
176-
177-
178-class BadPluralExpression(Exception):
179- """Local "escape hatch" exception for unusable plural expressions."""
180-
181-
182-def make_plural_function(expression):
183- """Create a lambda function for C-like plural expression."""
184- # Largest expressions we could find in practice were 113 characters
185- # long. 500 is a reasonable value which is still 4 times more than
186- # that, yet not incredibly long.
187- if expression is None or len(expression) > 500:
188- raise BadPluralExpression
189-
190- # Guard against '**' usage: it's not useful in evaluating
191- # plural forms, yet can be used to introduce a DoS.
192- if expression.find('**') != -1:
193- raise BadPluralExpression
194-
195- # We allow digits, whitespace [ \t], parentheses, "n", and operators
196- # as allowed by GNU gettext implementation as well.
197- if not re.match('^[0-9 \t()n|&?:!=<>+%*/-]*$', expression):
198- raise BadPluralExpression
199-
200- try:
201- function = gettext.c2py(expression)
202- except (ValueError, SyntaxError):
203- raise BadPluralExpression
204-
205- return function
206-
207-
208-def make_plurals_identity_map():
209- """Return a dict mapping each plural form number onto itself."""
210- return dict(enumerate(xrange(TranslationConstants.MAX_PLURAL_FORMS)))
211-
212-
213-def plural_form_mapper(first_expression, second_expression):
214- """Maps plural forms from one plural formula to the other.
215-
216- Returns a dict indexed by indices in the `first_formula`
217- pointing to corresponding indices in the `second_formula`.
218- """
219- identity_map = make_plurals_identity_map()
220- try:
221- first_func = make_plural_function(first_expression)
222- second_func = make_plural_function(second_expression)
223- except BadPluralExpression:
224- return identity_map
225-
226- # Can we create a mapping from one expression to the other?
227- mapping = {}
228- for n in range(1000):
229- try:
230- first_form = first_func(n)
231- second_form = second_func(n)
232- except (ArithmeticError, TypeError):
233- return identity_map
234-
235- # Is either result out of range?
236- valid_forms = range(TranslationConstants.MAX_PLURAL_FORMS)
237- if first_form not in valid_forms or second_form not in valid_forms:
238- return identity_map
239-
240- if first_form in mapping:
241- if mapping[first_form] != second_form:
242- return identity_map
243- else:
244- mapping[first_form] = second_form
245-
246- # The mapping must be an isomorphism.
247- if sorted(mapping.keys()) != sorted(mapping.values()):
248- return identity_map
249-
250- # Fill in the remaining inputs from the identity map:
251- result = identity_map.copy()
252- result.update(mapping)
253- return result
254+from lp.translations.utilities.pluralforms import (make_plurals_identity_map,
255+ plural_form_mapper)
256+
257
258
259 class POSyntaxWarning(Warning):
260
261=== added file 'lib/lp/translations/utilities/pluralforms.py'
262--- lib/lp/translations/utilities/pluralforms.py 1970-01-01 00:00:00 +0000
263+++ lib/lp/translations/utilities/pluralforms.py 2009-09-10 00:07:47 +0000
264@@ -0,0 +1,103 @@
265+# Copyright 2009 Canonical Ltd. This software is licensed under the
266+# GNU Affero General Public License version 3 (see the file LICENSE).
267+
268+import re
269+import gettext
270+from lp.translations.interfaces.translations import (
271+ TranslationConstants)
272+
273+
274+class BadPluralExpression(Exception):
275+ """Local "escape hatch" exception for unusable plural expressions."""
276+
277+def make_friendly_plural_forms(expression, pluralforms_count):
278+ """Return a dict's list of plural forms' examples from an expression."""
279+ expression_ = make_plural_function(expression)
280+ forms = {}
281+
282+ for n in range(1, 200):
283+ forms.setdefault(expression_(n), [])
284+ # Giving a limit of 6 examples per plural form. If all the forms were
285+ # contemplated, it stops.
286+ if len(forms[expression_(n)]) == 6 and len(forms) == pluralforms_count:
287+ break
288+ if len(forms[expression_(n)]) == 6:
289+ continue
290+ forms[expression_(n)].append(n)
291+
292+ return [{'form':k, 'examples':v} for (k,v) in forms.iteritems()]
293+
294+
295+def make_plural_function(expression):
296+ """Create a lambda function for C-like plural expression."""
297+ # Largest expressions we could find in practice were 113 characters
298+ # long. 500 is a reasonable value which is still 4 times more than
299+ # that, yet not incredibly long.
300+ if expression is None or len(expression) > 500:
301+ raise BadPluralExpression
302+
303+ # Guard against '**' usage: it's not useful in evaluating
304+ # plural forms, yet can be used to introduce a DoS.
305+ if expression.find('**') != -1:
306+ raise BadPluralExpression
307+
308+ # We allow digits, whitespace [ \t], parentheses, "n", and operators
309+ # as allowed by GNU gettext implementation as well.
310+ if not re.match('^[0-9 \t()n|&?:!=<>+%*/-]*$', expression):
311+ raise BadPluralExpression
312+
313+ try:
314+ function = gettext.c2py(expression)
315+ except (ValueError, SyntaxError):
316+ raise BadPluralExpression
317+
318+ return function
319+
320+
321+def make_plurals_identity_map():
322+ """Return a dict mapping each plural form number onto itself."""
323+ return dict(enumerate(xrange(TranslationConstants.MAX_PLURAL_FORMS)))
324+
325+
326+def plural_form_mapper(first_expression, second_expression):
327+ """Maps plural forms from one plural formula to the other.
328+
329+ Returns a dict indexed by indices in the `first_formula`
330+ pointing to corresponding indices in the `second_formula`.
331+ """
332+ identity_map = make_plurals_identity_map()
333+ try:
334+ first_func = make_plural_function(first_expression)
335+ second_func = make_plural_function(second_expression)
336+ except BadPluralExpression:
337+ return identity_map
338+
339+ # Can we create a mapping from one expression to the other?
340+ mapping = {}
341+ for n in range(1000):
342+ try:
343+ first_form = first_func(n)
344+ second_form = second_func(n)
345+ except (ArithmeticError, TypeError):
346+ return identity_map
347+
348+ # Is either result out of range?
349+ valid_forms = range(TranslationConstants.MAX_PLURAL_FORMS)
350+ if first_form not in valid_forms or second_form not in valid_forms:
351+ return identity_map
352+
353+ if first_form in mapping:
354+ if mapping[first_form] != second_form:
355+ return identity_map
356+ else:
357+ mapping[first_form] = second_form
358+
359+ # The mapping must be an isomorphism.
360+ if sorted(mapping.keys()) != sorted(mapping.values()):
361+ return identity_map
362+
363+ # Fill in the remaining inputs from the identity map:
364+ result = identity_map.copy()
365+ result.update(mapping)
366+ return result
367+