Refactor cache folder creation to its own function #149
Conversation
Move the code from `pooch.create` to `pooch.utils.make_local_storage`. The function joins the path, adds the version string, checks the environment variable, expands the user path, checks if the path is writable (now printing different messages if we can't create the folder vs can't create a file), and converts the string path to `pathlib.Path`. This will make it easier to reuse this part of the code is needed.
| message += "Use environment variable '%s' to specify another directory." | ||
| args += [env] | ||
|
|
||
| get_logger().warning(message, *args) |
There was a problem hiding this comment.
I realize that I'm the one who made this a warning when we switched over to using the logging API, but do you think it should become an error? If the cache directory isn't writable, the user will just hit another error right after when they try to download something. But the real cause is here and passing through with only a warning could just obscure that.
There was a problem hiding this comment.
I was thinking the same thing. If this is the case, the warning will pop up on import (usually when a package creates the Pooch instance) and an exception is only raised on Pooch.fetch. So if running on Jupyter/IPython there will be time to act and set the environment variable. But if running on a script then the error comes right after the warning and people might not see it.
In that case, it might be better to added the message to the error and re-raise it. What do you think?
There was a problem hiding this comment.
I was only thinking about the case of running it in a script and hadn't thought about jupyter. In any case I think that changing it from warning to error and re-raising the exception is the way to go.
There was a problem hiding this comment.
Agreed. I opened #150 to track this for a future PR instead of implementing it here to avoid changing 2 things at once. Thanks for the feedback @danshapero 👍
Move the code from
pooch.createtopooch.utils.make_local_storage.The function joins the path, adds the version string, checks the
environment variable, expands the user path, checks if the path is
writable (now printing different messages if we can't create the folder
vs can't create a file), and converts the string path to
pathlib.Path.This will make it easier to reuse this part of the code is needed.
This will pave the way for #147
Reminders:
make formatandmake checkto make sure the code follows the style guide.doc/api/index.rstand the base__init__.pyfile for the package.AUTHORS.mdfile (if you haven't already) in case you'd like to be listed as an author on the Zenodo archive of the next release.