Simplify the structure of the grass init module #7
Conversation
shutil.rmtree removes directories recursively, so no need for the wrapper function/classes.
The function is being used later in this file.
"clean_env()" is only being called once. Even if it gets called multiple times in the future, it is unlikely that it will be used with a different "gisrc".
Just keeping globals and the initialization stuff at the beginning of the module
This was messing up with the vim code folding.
Six is being pulled by matplotlib anyway, but for good measure, I also added it to the explicit requirements. This might require additional action on win/mac.
The comment is from 2015. If there were problems, they would have surfaced since then.
- GISBASE - CMD_NAME - GRASS_VERSION - LD_LIBRARY_PATH
The main motivation here is to avoid making "_()" calls at the module level which will hopefully let us remove the early "gettext.install()" call in a later commit.
With this commit: 1. we remove the "gettext.install()" call from the module level 2. we make sure that "set_language()" is the first step of the init process.
pmav99
force-pushed
the
simplify_grass_init
branch
from
8126150 to
1809611
Compare
There was a problem hiding this comment.
Thanks, this looks great to me. (I did previous refactoring of this file, although it is perhaps hard to believe that any was done at all, but believe me or see the history.) I'm asking only for minor changes, asking to consider couple of additional things, and providing context for some of the code. I'm open to how you actually want to submit the additional changes.
All these commits seem to compile and run, so there is nothing preventing us from keeping them except for the high number. I'm for detailed history, so either that or all commit messages (edited) in the combined commit.
| python-dev \ | ||
| unixodbc-dev \ | ||
| libnetcdf-dev \ | ||
| netcdf-bin \ | ||
| dpatch \ | ||
| libblas-dev \ | ||
| liblapack-dev \ | ||
| python-numpy | ||
|
|
||
| python-numpy |
There was a problem hiding this comment.
Whitespace change introducing no new line at the end of file, fix:
| python-numpy | |
| python-numpy | |
| # possibly same for GRASS_PROJSHARE and others but maybe not | ||
| # | ||
| # We need to simultaneously make sure that: | ||
| # - we get GISBASE from os.environ if it is defined (doesn't this mean that we are already |
There was a problem hiding this comment.
I agree. We can change that for 8.0 and even make some forward compatible changes in 7.x series. (just a comment, no change needed here)
There was a problem hiding this comment.
BTW, this related to the comment removed in 2c00830: We should be able to overpower a running G session by another one.
| @@ -168,14 +168,14 @@ def try_remove(path): | |||
| pass | |||
|
|
|||
|
|
|||
| def clean_env(gisrc): | |||
There was a problem hiding this comment.
The idea for the parameter here is that this is one of the functions which should eventually go to grass.script.setup to be (re)used when a session is started using the grass package from Python. (Maybe less important now with grass-session package for external uses of G, but still very useful for subsession dealing with different projections or mapsets.)
| @@ -1584,7 +1585,6 @@ def close_gui(): | |||
| env = gcore.gisenv() | |||
| if 'GUI_PID' not in env: | |||
| return | |||
| import signal | |||
There was a problem hiding this comment.
Perhaps minor speedup, but this is currently needed only for the GUI and in general we should minimize the overhead when grass.py is called in command line to do batch jobs. (import types is similar case, although not related to GUI vs batch) Just a note. I'm leaving it up to you.
| @@ -58,51 +58,45 @@ | |||
| print("Default locale not found, using UTF-8") # intentionally not translatable | |||
|
|
|||
|
|
|||
| def decode(bytes_, encoding=None): | |||
| def decode(bytes_, encoding=ENCODING): | |||
| """Decode bytes with default locale and return (unicode) string | |||
| Adapted from lib/python/core/utils.py | |||
There was a problem hiding this comment.
Should say "Adapted from lib/python/script/utils.py" (also for the other function)
| @@ -172,13 +173,6 @@ def try_remove(path): | |||
| pass | |||
|
|
|||
|
|
|||
| def try_rmdir(path): | |||
There was a problem hiding this comment.
I guess the idea was to have something like in grass.script.utils and write try_rmdir(tmpdir) instead of lambda: shutil.rmtree(tmpdir, ignore_errors=True), but I don't see clear rule here (brevity vs std library use). As for removing the Cleaner class, I agree, code was much simplified, no need for it anymore (see my fixes for the multiple/incorrect tmp dirs).
| @@ -30,12 +30,12 @@ sudo apt-get install --no-install-recommends \ | |||
| libxmu-dev \ | |||
| python \ | |||
| python-wxgtk3.0 \ | |||
| python-six \ | |||
There was a problem hiding this comment.
Please make sure the additional action is actually taken for the standalone win installer, OSGeo4W, and Mac (installer and any brew) by testing or raising this on the ML or in the Trac issue.
There was a problem hiding this comment.
This series of commits are all applied to
lib/init/grass.py. The main goal is to simplify the structure of the module by moving everything that is related to the actual initialization process from the module level tomain()but there are improvements as well (e.g. code simplifications, PEP8 etc)The changes have been tested on:
If required, I can make a new PR with a single squashed commit, but I think that reviewing the changes, will be easier here.