Simplify retrieval of cache and config directories by anntzer · Pull Request #11398 · matplotlib/matplotlib

anntzer · 2018-06-07T17:20:16Z

PR Summary

In get_home, we shouldn't check for $TMP, but instead let this case
fall through to _create_config_or_cache_dir.
When checking for $XDG_{CONFIG,CACHE}_HOME and $MPLCONFIGDIR,
consider empty strings as equivalent to unset (which is standard
behavior, e.g. one could write XDG_CONFIG_HOME= python ... and
expect things to behave as if XDG_CONFIG_HOME was indeed unset).
The logic in _get_config_or_cache_dir can greatly simplified: try to
create a candidate, generating it on the way if necessary; if it
cannot be created or is not writable, fallback to a temporary
directory.
Simplify caching of directories retrieval.

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

efiring

I haven't actually tested all this for functionality, but it looks plausible, and the reduction in LOC is nice. Readability needs improvement, even though it will require a few lines of comment and docstring.

efiring · 2018-06-24T18:34:07Z

lib/matplotlib/__init__.py

-    """
-    return a callable function that wraps func and reports its
-    output through logger
+def _logged_cached(fmt, func=None):


This needs a docstring to explain what it does.

efiring · 2018-06-24T18:38:54Z

lib/matplotlib/__init__.py

-    output through logger
+def _logged_cached(fmt, func=None):
+    if func is None:
+        return functools.partial(_logged_cached, fmt)


I think this needs a comment to explain what it is doing, and how. I read the functools.partial doc, but I'm still completely baffled by these lines. I dimly recollect that you already explained something like this somewhere else, but if so, it didn't stick in my brain.

Would lambda func: _logged_cached(fmt, func) look better for you? That's basically the same thing.

Or I can even implement the decorator using more nested functions, but I think the flatter pattern is usually preferred.

It's probably fine the way it is; I'm just looking for a hopefully short comment that will explain it. I suspect the conceptual difficulty comes from the fact that the simpler and more common use of decorators is without arguments. What your usage is doing is first executing the _logged_cashed(fmt) to yield the real decorator function object that gets applied to the func that follows it. That real decorator remembers and can use fmt, and is a function that takes only one argument, which is now 'func', as expected for a decorator. A key point is that the initial function execution to yield the real decorator occurs before the application by the @.

Maybe insert a comment:

if func is None: # Return the decorator function object. ....

Or maybe that is overkill. I think I understand it now, and will probably recognize it in the future; I actually like it. It's just a question of whether a short comment will save someone else some time in understanding how this works. I'm not sure.

Added a short comment (and at another place where I used the same pattern: cbook._define_aliases).

efiring · 2018-06-24T18:50:22Z

lib/matplotlib/__init__.py

-            path = os.path.join(path, '.config')
-    return path
+    return (os.environ.get('XDG_CONFIG_HOME')
+            or (Path(get_home(), ".config")


Is there a reason for returning a string in the first case and a Path object in the second? Or is it just that for the way the output is used it doesn't matter, and you like using pathlib instead of os.path?

It's true that it doesn't matter, but I changed it to return str every time -- there's no reason to be inconsistent.

efiring · 2018-06-24T18:59:43Z

lib/matplotlib/__init__.py

+        if sys.platform.startswith(('linux', 'freebsd')) and xdg_base
+        else Path(get_home(), ".matplotlib")
+        if get_home()
+        else None)


These chained "one-line" ternaries take a little getting used to... but maybe they are OK in the end.

I don't find this particularly easy to read either, and I'm not sure what the advantage is other than saving a couple of lines of code.

Flattened this one. Left the one for _get_xdg_foo_dir as I think they're still more readable(?)

efiring · 2018-06-24T19:01:46Z

lib/matplotlib/testing/compare.py

 from matplotlib.testing.exceptions import ImageComparisonFailure
-from matplotlib import _png
-from matplotlib import _get_cachedir
+from matplotlib import _png, cbook


Duplicate import of cbook

1. In get_home, we shouldn't check for $TMP, but instead let this case fall through to _create_config_or_cache_dir. 2. When checking for $XDG_{CONFIG,CACHE}_HOME and $MPLCONFIGDIR, consider empty strings as equivalent to unset (which is standard behavior, e.g. one could write `XDG_CONFIG_HOME= python ...` and expect things to behave as if `XDG_CONFIG_HOME` was indeed unset). 3. The logic in _get_config_or_cache_dir can greatly simplified: try to create a candidate, generating it on the way if necessary; if it cannot be created or is not writable, fallback to a temporary directory.

anntzer force-pushed the gethome branch from e4ae288 to 6778e4e Compare June 7, 2018 18:18

anntzer mentioned this pull request Jun 23, 2018

Some more fixes for the verbose -> logging switch. #9761

Closed

6 tasks

efiring requested changes Jun 24, 2018

View reviewed changes

anntzer force-pushed the gethome branch 2 times, most recently from 44aaa1b to 868eeb1 Compare June 24, 2018 22:21

anntzer added 2 commits June 25, 2018 01:03

Simplify caching of directories retrieval.

5840fee

anntzer force-pushed the gethome branch 2 times, most recently from 798f837 to 18da838 Compare June 24, 2018 23:27

efiring approved these changes Jun 25, 2018

View reviewed changes

Fixes following PR review.

8be4eae

anntzer force-pushed the gethome branch from 18da838 to 8be4eae Compare June 25, 2018 11:40

timhoffm approved these changes Jul 9, 2018

View reviewed changes

timhoffm merged commit 85f0dd6 into matplotlib:master Jul 9, 2018

QuLogic added this to the v3.0 milestone Jul 9, 2018

anntzer deleted the gethome branch July 10, 2018 08:21

anntzer mentioned this pull request Jul 11, 2018

Simplify (quite a bit...) _preprocess_data #10928

Merged

6 tasks

Uh oh!

Conversation

anntzer commented Jun 7, 2018

PR Summary

PR Checklist

Uh oh!

efiring left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants