Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
da0b7fd
to0efa52f
CompareIt'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, seehttps://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 ( |
efiring commentedNov 14, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 Nonedefget_home():"""Find user's home directory if possible. Otherwise, returns None. """if_user_homeisnot0:return_user_home_user_home=Noneifsix.PY2andsys.platform=='win32':path=os.path.expanduser(b"~").decode(sys.getfilesystemencoding())else:path=os.path.expanduser("~")ifos.path.isdir(path):_user_home=pathelse:forevarin ('HOME','USERPROFILE','TMP'):path=os.environ.get(evar)ifpathisnotNoneandos.path.isdir(path):_user_home=pathbreak_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 feelvery 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. Ithink 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?
I pushed a new version which should be hopefully a bit clearer and better documented. |
Technically, this PR changes the behavior of get_home(), get_cachedir(),etc. making them insensitive to changes in the relevant environmentvariables *after* matplotlib is imported. In practice I strongly doubtthat we supported that use case to begin with (because the values areprobably cached in other places); I am also happy with saying "don't dothis" until someone brings up a good reason to change the cache dir inthe middle of the program...
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
variablesafter 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