#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
Reporter: pmav99 | Owner: grass-dev@…
Type: enhancement | Status: closed
Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: fixed | Keywords:
CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------
Comment (by pmav99):
Disclaimer: The real goal of this refactoring was to get rid of
{{{os.getenv("GISBASE")}}} calls at the module level. This is a
prerequisite for #3772. The scripts and the GUI code didn't really need to
be touched, but I did them too for uniformity.
Replying to [comment:4 wenzeslaus]:
> The things in lib/python are potentially (and actually) used as Python
modules by other applications or scripts. Thus from
https://pymotw.com/2/gettext/index.html#module-localization, it is
actually the second option which applies:
>
> > //For a library, or individual module, modifying `__builtins__` is not
a good idea because you don’t know what conflicts you might introduce with
an application global value. You can import or re-bind the names of
translation functions by hand at the top of your module.//
I really like it when things are explicit. I dislike the idea of injecting
things into builtins and I do found the convention of using {{{_()}}} as
an alias for {{{gettext.gettext()}}} rather unfortunate. Consequently, I
don't have any objections with getting rid of {{{gettext.install()}}}
and/or even {{{_()}}}.
Regardless, I am not sure that GRASS was actually following Doug Helman's
advice. More specifically, almost all GRASS modules used to import the
{{{grass}}} library before actually calling {{{gettext.install()}}} on
their own ([https://github.com/GRASS-GIS/grass-
ci/pull/9/commits/0983ca068d6f7363d2fede26f77b8aed76c87bcc source]). The
{{{grass}}} library was already calling {{{gettext.install}}}
([https://github.com/GRASS-GIS/grass-
ci/pull/9/commits/b589754aa497ac8ad067ed8258b12caaa0bf1e17#diff-
15e2e35efef103adc7f77c0a3afe4a63 source]). {{{grass.temporal}}} was doing
the same for no apparent reason though, since it was just re-registering
the "grasslibs" domain ([https://github.com/GRASS-GIS/grass-
ci/pull/9/commits/b589754aa497ac8ad067ed8258b12caaa0bf1e17#diff-
e36b6b10b67c75c0d7c159b273854ef1 source]). Anyway, the end result is that
most of the time, if not always, as soon as you imported the {{{grass}}}
lib, the {{{builtins}}} were modified anyway.
In other words, I don't think that the proposed patch really changes what
the {{{grass}}} library was doing. The builtin injection continues to
happen just like it used before.
That being said, I am completely open to any improvements. I am also
perfectly fine with following Mr. Helman's advice (i.e. by adding explicit
imports in each and every {{{grass}}} lib module) if someone wants to
contribute that. But IMHV the proposed implementation is cleaner than the
previous one.
> So far we were relatively successful in keeping the GUI separate
(whether for performance or code organization reasons).
I don't really see any coupling of the GUI code with the {{{grass}}}
library. All the {{{grass}}} library does is registering the "grasswxpy"
domain. It doesn't import anything.
TBH, I did consider moving the {{{gettext.install("grasswxpy", ...)}}}
code inside {{{./gui/wxpython}}}, but the way it is structured doesn't
allow to do that cleanly and I didn't want to make changes there too. It
is doable though.
> Finally, is there any overhead in centralizing the tr string
installation?
I haven't measured it. Obviously there is some overhead. Previously when
you were importing {{{grass}}} for the first time in a process, you were
registering 2 domains. "grasslibs" + "grassmods" when using the CLI and
"grasslibs" + "grasswxpy" when using the GUI. Now 3 domains are being
registered. So there is overhead.
TBH I haven't read the whole {{{gettext}}} module line by line, but I
think that the gist of {{{gettext.install()}}} is
[https://github.com/python/cpython/blob/ead15795986972690217e52087eb946b8a5bbcda/Lib/gettext.py#L512-L552
these lines]. Line 552 calls the method that does the builtin injection
(check lines 314-316) while line 551 calls the function that registers the
domain in a "private" dictionary named {{{_translations}}}. AFAI can tell
from just reading the code, the only overhead to really speak of is the
parsing of the {{{*.mo}}} files which in GRASS are a few hundred KBs. On
SSDs this overhead should be negligible while on mechanical drives the
bottleneck is not going to be the code anyway. Re-registering a domain
should incur any overhead since the results are cached.
That being said, if you feel that it is necessary, it should be possible
to avoid registering "grasswxpy" and "grassmods" inside grass lib. E.g. by
adding a couple of functions in {{{./lib/python/__init__.py}}}:
{{{
# ...
_LOCALE_DIR = os.path.join(os.getenv("GISBASE"), 'locale')
gettext.install('grasslibs', _LOCALE_DIR)
def register_grassmods():
gettext.install('grassmods', _LOCALE_DIR)
def register_grasswxpy():
gettext.install('grasswxpy', _LOCALE_DIR)
}}}
and making sure that they are imported by each and every module that needs
them. But I am curious if the impact is that significant to make it worth
it.
--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:5>
GRASS GIS <https://grass.osgeo.org>