[GRASS-dev] [GRASS GIS] #2425: Import translation function instead of using a buildin

GDAL · September 17, 2014, 1:35am

#2425: Import translation function instead of using a buildin
-----------------------------------------------------+----------------------
Reporter: wenzeslaus | Owner: grass-dev@…
Type: task | Status: new
Priority: normal | Milestone: 7.1.0
Component: Translations | Version: svn-releasebranch70
Keywords: python, underscore, _, gettext, doctest | Platform: All
Cpu: Unspecified |
-----------------------------------------------------+----------------------
''Here are my notes about this issue since I was forgetting about it.''

On some occasions, some of GRASS functions does not work because of
translation function (`_` - underscore). The problem occurs on some
strange conditions and when you are using Python
[https://docs.python.org/2/library/doctest.html doctest] which we are
using more and more.

There is already several workarounds in wxGUI and also testing framework
as
[wiki:GSoC/2014/TestingFrameworkForGRASS#Findingandrunningthetestmodules
described here]:
> `doctests` currently don't work with grass.script unless you call a
[source:grass/trunk/gui/wxpython/core/toolboxes.py?rev=60218#L630 set of
functions] to deal with function _ (underscore) because of installing
translate function as buildin _ function while _ function is used also in
doctest.

Grep for function `do_doctest_gettext_workaround` to see definition and
how it is used.

I already did this change for wxGUI more then a year ago and it seems to
work. It was necessary because there was some problem with not translated
files and this was only way to fix it besides going against documentation.

For now I don't know how `lib/python`, `scripts`, and `temporal` differs
in translations, so this should be clarified before changing it.

See comment:5:ticket:1739 for deep discussion of the topic.

For wxGUI this was implemented in the r57219 and r57220 as a result of
#1739 and other investigation.

This might be a blocker for some solutions of #2142. The new method should
be flexible allowing to obtain translations from two sources
simultaneously.

Basically, it is necessary to remove all occurrences of:

{{{
gettext.install('grasswxpy', os.path.join(os.getenv("GISBASE"), 'locale'),
unicode = True)
}}}

By one code based on this line in some module:

{{{
_ = gettext.translation('grasswxpy', os.path.join(os.getenv("GISBASE"),
'locale')).ugettext
}}}

The actual implementation must be more complicated
(changeset:57220#file0).

Then each file, depending on the name and placement of translation
function, must import:
{{{
from grass.script.utils import _
from grass.script.translations import _
from grass.*.utils import _
from grass.*.translations import _
from grass.translations import _
}}}

Probably `grass.translations` is the best since in GUI a module inside
some other package is creating dependencies (changeset:57219#file10,
changeset:57219#file13).

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2425>
GRASS GIS <http://grass.osgeo.org>

GDAL · September 17, 2014, 9:20pm

#2425: Import translation function instead of using a buildin
-----------------------------------------------------+----------------------
Reporter: wenzeslaus | Owner: grass-dev@…
Type: task | Status: new
Priority: normal | Milestone: 7.1.0
Component: Translations | Version: svn-releasebranch70
Keywords: python, underscore, _, gettext, doctest | Platform: All
Cpu: Unspecified |
-----------------------------------------------------+----------------------

Comment(by glynn):

Replying to [ticket:2425 wenzeslaus]:

> Basically, it is necessary to remove all occurrences of:
>
{{{
gettext.install('grasswxpy', os.path.join(os.getenv("GISBASE"), 'locale'),
unicode = True)
}}}
>
> By one code based on this line in some module:
>
{{{
_ = gettext.translation('grasswxpy', os.path.join(os.getenv("GISBASE"),
'locale')).ugettext
}}}

I'd suggest that what's actually "necessary" is to fix whatever can't cope
with "_" being a built-in function.

It shouldn't be necessary to explicitly import "_" into each and every
file. This is why Python has a (mutable) built-in namespace.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2425#comment:1>
GRASS GIS <http://grass.osgeo.org>

GDAL · September 17, 2014, 10:02pm

#2425: Import translation function instead of using a buildin
-----------------------------------------------------+----------------------
Reporter: wenzeslaus | Owner: grass-dev@…
Type: task | Status: new
Priority: normal | Milestone: 7.1.0
Component: Translations | Version: svn-releasebranch70
Keywords: python, underscore, _, gettext, doctest | Platform: All
Cpu: Unspecified |
-----------------------------------------------------+----------------------

Comment(by wenzeslaus):

Replying to [comment:1 glynn]:
> Replying to [ticket:2425 wenzeslaus]:
>
> > Basically, it is necessary to remove all occurrences of:
> >
> {{{
> gettext.install('grasswxpy', os.path.join(os.getenv("GISBASE"),
'locale'), unicode = True)
> }}}
> >
> > By one code based on this line in some module:
> >
> {{{
> _ = gettext.translation('grasswxpy', os.path.join(os.getenv("GISBASE"),
'locale')).ugettext
> }}}
>
> I'd suggest that what's actually "necessary" is to fix whatever can't
cope with "_" being a built-in function.

That's a good point but both doctest and gettext are part of Python and
they are still not compatible. I would say that they don't want to change
it. The issue is known for a long time and by a lot of people:
  * https://mail.python.org/pipermail/python-list/2004-February/248035.html
([http://bytes.com/topic/python/answers/28870-problem-mixing-doctest-
gettext-_ other part of discussion]
  * http://sheep.art.pl/Gettext%20and%20Doctest%20Together%20in%20Python
  * https://github.com/nose-devs/nose/issues/393
  * (however, no
[http://bugs.python.org/issue?@filter=status&@filter=components&components=4&status=1&@columns=id,activity,title,status&@sort=-activity
Python issue] found)

A lot of people/projects went the way of using explicit import (rather
then (implicit) buildin), most notably Django (although I'm aware of some
recent strange Django decisions):
* https://docs.djangoproject.com/en/dev/topics/i18n/translation/

> It shouldn't be necessary to explicitly import "_" into each and every
file. This is why Python has a (mutable) built-in namespace.

Yes, that's true, Python allows that but I don't think that it is a good
practice. Of course, gettext is a bit special but this does not mean that
it will save it from the risks of using buildin (doctest is probably using
the buildin for the very same reason, it considers itself special).

The other motivation is that wxGUI needed some change (since the former
state was not working) and the only two solutions we found were putting
`install` call to each file (which is agiast buildin and gettext
documentation) or using explicit imports.

I think we should you the same practice everywhere but what remained me
about this issue was that I have seen in some Python editor an error
message which was saying that `_(...)` does not take any arguments (or
something like that, I was not able to get or reproduce the message). This
is the kind of message is the same as given by doctest, so I guess
explicit imports would solve both issues.

The number of imports is high for most of the files (with the exception of
simple scripts), so I don't consider this as a huge issue.

We should decide this some day. I wouldn’t do the change for 7.0, but for
trunk/7.1 it should be safe.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2425#comment:2>
GRASS GIS <http://grass.osgeo.org>

GDAL · September 17, 2014, 11:34pm

#2425: Import translation function instead of using a buildin
-----------------------------------------------------+----------------------
Reporter: wenzeslaus | Owner: grass-dev@…
Type: task | Status: new
Priority: normal | Milestone: 7.1.0
Component: Translations | Version: svn-releasebranch70
Keywords: python, underscore, _, gettext, doctest | Platform: All
Cpu: Unspecified |
-----------------------------------------------------+----------------------

Comment(by glynn):

Replying to [comment:2 wenzeslaus]:

> That's a good point but both doctest and gettext are part of Python and
they are still not compatible. I would say that they don't want to change
it. The issue is known for a long time and by a lot of people:

In which case, I would say that doctest loses. Either come up with a
workaround for doctest, or live without it. It isn't acceptable to require
every programmer to jump through hoops for the sake of doctest.

> The other motivation is that wxGUI needed some change (since the former
state was not working) and the only two solutions we found were putting
`install` call to each file (which is agiast buildin and gettext
documentation) or using explicit imports.

I believe that wxPython has its own "_" related to its own I18N subsystem.
If possible, I would prefer to "fix" wxPython. And if that isn't possible,
I would prefer that any sub-optimal solutions don't end up infecting non-
wxGUI code.

> I think we should you the same practice everywhere but what remained me
about this issue was that I have seen in some Python editor an error
message which was saying that `_(...)` does not take any arguments (or
something like that, I was not able to get or reproduce the message). This
is the kind of message is the same as given by doctest, so I guess
explicit imports would solve both issues.

This sounds like the interactive shell using "_" as a variable to hold the
result of the last expression:
{{{
> 2 + 3
5
> _
5
> __builtins__._
5
}}}

It may be that doctest is also using "_" in this way in order to
faithfully mimic the behaviour of the interactive shell. Actually, there's
no "may" about it; the behaviour is implemented by
[https://docs.python.org/2.7/library/sys.html#sys.displayhook
sys.displayhook]:
{{{
sys.displayhook(value)

If value is not None, this function prints it to sys.stdout, and saves
it in __builtin__._.

sys.displayhook is called on the result of evaluating an expression
entered in an interactive Python session. The display of these values can
be customized by assigning another one-argument function to
sys.displayhook.
}}}

doctest.py explicitly restores this function to its initial value (from
[https://docs.python.org/2.7/library/sys.html#sys.__displayhook__
sys.__displayhook__]) while executing the tests:
{{{
         # Make sure sys.displayhook just prints the value to stdout
         save_displayhook = sys.displayhook
         sys.displayhook = sys.__displayhook__
}}}

Indeed, trying to debug code which uses _() by pasting it into an
interactive session often fails because the shell assigns `__builtins__._`
every time you enter a statement. But that's just how it is; the function
has to be called _() so that xgettext can identify strings for
translation.

One possibility is to replace both sys.displayhook and
`sys.__displayhook__` with something which preserves `__builtin__._` while
running doctest. Obviously, any examples which rely upon the interpreter
setting "_" won't work, but that's easy enough to avoid (and in any case,
they won't work if the code use "from whatever import _", because
variables in the current module override built-ins).

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2425#comment:3>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 2, 2014, 12:03am

#2425: Import translation function instead of using a buildin
-----------------------------------------------------+----------------------
Reporter: wenzeslaus | Owner: grass-dev@…
Type: task | Status: new
Priority: normal | Milestone: 7.1.0
Component: Translations | Version: svn-releasebranch70
Keywords: python, underscore, _, gettext, doctest | Platform: All
Cpu: Unspecified |
-----------------------------------------------------+----------------------

Comment(by wenzeslaus):

I still think that not using `_` as a buildin for translations is a good
idea adopted by other projects such as Django (comment:5:ticket:1739).

If user is trying something in Python command line (in system terminal, in
wxGUI or elsewhere), he or she will get strange errors in case the Python
functions will print some warnings, errors or any translatable text. And
this is wrong I would say.

{{{
>>> from grass.script.core import run_command
>>> run_command('g.region', rast_='elevation')
0
>>> run_command('g.region', _rast='elevation')
Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
   File ".../etc/python/grass/script/core.py", line 355, in run_command
     ps = start_command(*args, **kwargs)
   File ".../etc/python/grass/script/core.py", line 334, in start_command
     args = make_command(prog, flags, overwrite, quiet, verbose, **options)
   File ".../etc/python/grass/script/core.py", line 283, in make_command
     warning(_("To run the module add underscore at the end"
TypeError: 'int' object is not callable
>>> _
0
}}}

It will work in IPython, however:

{{{
In [1]: from grass.script.core import run_command

In [2]: run_command('g.region', rast_='elevation')
Out[2]: 0

In [3]: run_command('g.region', _rast='elevation')
WARNING: To run the module add underscore at the end of the option <rast>
          to avoid conflict with Python keywords. Underscore at the
          beginning is depreciated in GRASS GIS 7.0 and will be removed in
          version 7.1.
Out[3]: 0

In [4]: _
Out[4]: <bound method NullTranslations.gettext of
<gettext.NullTranslations instance at 0x7ff801cc45a8>>
}}}

Anyway, using explicit import of `_` rather then "install" to buildins
would solve not only doctest and Python interactive interpreter but would
also satisfy number of code checkers which complain about undefined `_`
symbol. Moreover, "explicit is better than implicit" is from Zen of Python
([http://legacy.python.org/dev/peps/pep-0020/ PEP20]).

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2425#comment:4>
GRASS GIS <http://grass.osgeo.org>