[Freeipa-devel] [PATCH 66] Replace broken i18n shell test with Python test

[Petr Viktorin]

On 03/23/2012 10:33 PM, John Dennis wrote:
> Attached is is a new patch addressing Petr's review comment as well as
> some sample output for illustration purposes (I suggest you review the
> example.txt).
>
> Rather than wading through a log of dialog in the previous emails let me
> address Petr's issues and how I addressed them.
>
> The option parsing is totally reworked (see example.txt). Not only is
> optparse being used, but the options have been simplified and appear in
> groups. The script can do a variety of different things so you must
> specify the mode you want it to run in. I don't see an obvious candidate
> for a default mode, plus most users will run it as a make target anyway
> obviating the need to specify any command line options.
>
> I logically divided the validation checks, one for checking the English
> source strings (pot file) and one for checking the translations (po
> files). These represent two of the three possible modes of operations.
>
> Petr's has said several times that he would prefer the English sources
> strings be checked by other tests. I presume he is referring to the unit
> tests, but we do not have any such unit test nor are we likely to (at
> least not for string validation). The reason is you have to have
> available the set of strings from the sources which have been marked for
> translation. It's a somewhat complicated process to produce that set of
> strings and that logic are part of the install/po area, namely the
> generation of the pot file. This is the only logical place I can see for
> performing tests and/or validation on (translatable) source strings. I
> do not envision the validation ever being part of a unit test, it is
> functionally very different. There is a good argument for generating a
> new pot file and validating it on every code check-in. This patch does
> not do that, instead it's done when the pot file is generated manually,
> but it would be trival to add that feature once this framework is in place.
>
> However a good case could be made for the gettext test currently
> implemented in test_i18n.py being invoked as a unit test but there would
> be some structural work needed to make that happen (e.g. assuring the
> test mo file is generated and pointed to and calling things through IPA
> Gettext classes and having the framework run in the context of that
> locale). But we've never had a failure in that area yet so I'd be
> inclined to defer that at the moment, open a new ticket to integrate the
> gettext test in test_i18n.py with the test_text.py in the unit tests and
> consider that a separate work item so as not let it interfere with
> getting this committed. Being able to run the gettext test in the
> translation directory is valuable none-the-less. It would be better if
> it were fully integrated with our Gettext classes in text.py but that is
> best left as another task.
>
> By dividing the validation checks between the pot file and po files I
> think I addressed the majority of Petr's concerns over the validation of
> source strings. Petr also alluded Transifex errors as if Transifex would
> somehow catch the translation problems, but no such error checking
> exists (actually I'm considering giving this code to upstream Transifex,
> they're entirely Python based and they might find parts of it useful)
>
> But wait there's more ...
>
> Some of the validations were producing false positives because the
> validator could not determine whether some uses of $ and % were valid or
> mangled syntax. So I added a pedantic flag. It's off by default and
> skips checking for errors which are unlikely to be present and are
> difficult to check. You can run the validation with --pedantic as a
> safeguard and interpret the results. I thought this was preferred to
> removing the ability to perform the checks whatsoever.
>
> I also added a --show-strings flag so along with a error message and
> location it will output the offending string(s). This if off by default
> for brevity purposes but is useful for those having to make fixes or
> just to better understand the nature of the error.
>
> I tried to make the error output a bit less verbose with better formatting.
>
> I updated .gitignore to ignore the generated test files.
>

Great! Just two issues now.

According to the style guide, we do allow %s in a string iff it only 
appears once. The reason for named substitutions is that the word order 
can be changed, providing context is just secondary.
Why does the checker report messages with a single %s as errors?

You've removed test_lang from Makefile.in, but test and .PHONY still 
mention it.

-- 
Petr³