[GRASS-dev] [GRASS GIS] #3790: Cleanup gettext usage for python code

[GRASS GIS]

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