-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Some more fixes for the verbose -> logging switch. #9761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
da0b7fd
to
0efa52f
Compare
It's not clear to me what this is "fixing"--is the point of it the change in behavior you describe above, the freezing of the returned value? Furthermore, the |
get_home and friends cannot be properties because they are not methods (well, see https://www.python.org/dev/peps/pep-0549/, not that it matters for us). The point is that the previous implementation checked whether get_home (etc.) has already been called; regardless of whether it has already been called, the internal ( Instead, the proposed implementation first wraps the internal implementation ( |
Yes, I realized right after pressing "comment" that I didn't really mean "property"--but rather the sort of structure used with a property getter: _user_home = 0 # dummy flag prior to first call, since we can't use None
def get_home():
"""Find user's home directory if possible.
Otherwise, returns None.
"""
if _user_home is not 0:
return _user_home
_user_home = None
if six.PY2 and sys.platform == 'win32':
path = os.path.expanduser(b"~").decode(sys.getfilesystemencoding())
else:
path = os.path.expanduser("~")
if os.path.isdir(path):
_user_home = path
else:
for evar in ('HOME', 'USERPROFILE', 'TMP'):
path = os.environ.get(evar)
if path is not None and os.path.isdir(path):
_user_home = path
break
_log.log(logging.INFO, '$HOME=%s', _user_home)
return _user_home (Untested, probably can be written better.) Advantage: everything is explicit and in one place; it can be read without having to track down the decorators and figure out how they work. |
I don't feel very strongly about the PR (I think the current situation is not so clear, your approach of making everything explicit would work too). I just realized why the PR name was confusing: as mentioned in the original message I was initially planning to also fix subprocess calls (by moving the log to matplotlib.compat.subprocess.*, rather than having to repeat them every time), but I realized this would conflict with my other PR (on executable resolution) so I decided to leave that out for now, and ended up with a somewhat partial PR. |
Why we are calculating paths on demand rather not on module import? |
They are effectively computed at import time, the functions are only there to factor out common code and avoid polluting the module namespace with intermediate variables. |
lib/matplotlib/__init__.py
Outdated
@@ -578,7 +558,21 @@ def checkdep_usetex(s): | |||
return flag | |||
|
|||
|
|||
def _get_home(): | |||
def _log_result(fmt, level="INFO", func=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to use this, it could use a comment along the lines of the explanation in the comments above. I understand it now, sort of, but would never have understood it if it wasn't explained, just due to an unfamiliarity w/ functools. OTOH, I can follow @efiring code. I think the advantage here over @efiring is that you've made it so all the different calls don't need to have the same logic to avoid duplicate system calls. Does that make the name a bit of a misnomer? The real purpose is the wrapping, isn't it?
0efa52f
to
ce0951c
Compare
I pushed a new version which should be hopefully a bit clearer and better documented. |
ce0951c
to
86f2531
Compare
Technically, this PR changes the behavior of get_home(), get_cachedir(), etc. making them insensitive to changes in the relevant environment variables *after* matplotlib is imported. In practice I strongly doubt that we supported that use case to begin with (because the values are probably cached in other places); I am also happy with saying "don't do this" until someone brings up a good reason to change the cache dir in the middle of the program...
86f2531
to
eaa1207
Compare
Superseded (essentially) by #11398. |
followup to #9313. would also like to rework logging of subprocess calls (basically, provide a logging wrapper in matplotlib.compat.subprocess instead of doing it again and again) but that'll happen after a decision is reached on #9639.
Technically, this PR changes the behavior of get_home(), get_cachedir(),
etc. making them insensitive to changes in the relevant environment
variables after matplotlib is imported. In practice I strongly doubt
that we supported that use case to begin with (because the values are
probably cached in other places); I am also happy with saying "don't do
this" until someone brings up a good reason to change the cache dir in
the middle of the program...
PR Summary
PR Checklist