Providing custom cache directory name through ini option (issue #2543) #2551
Conversation
_pytest/cacheprovider.py
Outdated
| @@ -14,7 +14,8 @@ | |||
| class Cache(object): | |||
| def __init__(self, config): | |||
| self.config = config | |||
| self._cachedir = config.rootdir.join(".cache") | |||
| cache_dirname = config.getini("cache_dirname") | |||
| self._cachedir = config.rootdir.join(cache_dirname) | |||
There was a problem hiding this comment.
path.join will deconstruct absolute arguments into relative paths
>>> py.path.local().join('/bin')
local('/home/ronny/Projects/pytest-dev/pytest/bin')
>>> py.path.local().join('/bin', abs=1)
local('/bin')
There was a problem hiding this comment.
to bring this further, people might also use ~ as shortcut form home, and py.path doesnt autoresolve those
so correctness unfortunately needs a few more tests and cases handled
There was a problem hiding this comment.
I agree, this option is not for setting cache path. It is for change default name of cache directory, wich is created in rootdir.
ini option for setting cache path is so more difficult feature.
There was a problem hiding this comment.
@RockBomber good point, in that case, how about warn/fail in case its an absolute path or it starts with ~ until someone implements a more complete solution
There was a problem hiding this comment.
It was also my understanding that this was supposed to be relative to the rootdir.
I'm curious, why do you guys say it would be more difficult to support absolute paths? Can't we assume:
- If relative path, relative to
rootdir; - If absolute path, just use it as it is;
I think this might be an interim solution for #1089 for example. We might even consider calling os.path.expandvars on the variable, which would allow users to specify $XDG_CONFIG_DIR as the cache directory.
There was a problem hiding this comment.
Well, I'll try to add an appropriate warning or error message.
There was a problem hiding this comment.
Hmm I'm not sure, unless I'm missing something (and please clarify me if I am), wouldn't be just as easy to support both relative and absolute paths as I mentioned above?
I like the idea of supporting relative and absolute paths because we can eventually change the default to $XDG_CACHE_DIR as discussed in #1089.
_pytest/cacheprovider.py
Outdated
|
|
||
|
|
||
| class Cache(object): | ||
| def __init__(self, config): | ||
| self.config = config | ||
| self._cachedir = config.rootdir.join(".cache") | ||
| cache_dirname = config.getini("cache_dirname") |
There was a problem hiding this comment.
I would rather name this cache_dir instead so we can support absolute paths in the future.
_pytest/cacheprovider.py
Outdated
| cache_dirname = config.getini("cache_dirname") | ||
| if _isabs(_expanduser(cache_dirname)): | ||
| raise ValueError("cache dirname must be relative path, not absolute") | ||
| self._cachedir = config.rootdir.join(cache_dirname) |
There was a problem hiding this comment.
I suggest to create a staticmethod Cache.cache_dir_from_config(config) which contains this logic; this would enhance the readability and also allow you to unittest the logic inside it more easily.
doc/en/customize.rst
Outdated
| @@ -262,3 +262,11 @@ Builtin configuration file options | |||
|
|
|||
| This tells pytest to ignore deprecation warnings and turn all other warnings | |||
| into errors. For more information please refer to :ref:`warnings`. | |||
|
|
|||
| .. confval:: cache_dirname | |||
|
|
|||
testing/test_cache.py
Outdated
| """) | ||
| testdir.makepyfile(test_errored='def test_error():\n assert False') | ||
| testdir.runpytest() | ||
| assert os.path.exists('custom_cache_dirname') |
There was a problem hiding this comment.
I'm surprised this is passing... does testdir change the cwd of the test?
You might be more explicit here either way:
assert testdir.tmpdir.join('custom_cache_dirname').isdir()
testing/test_cache.py
Outdated
| assert os.path.exists('custom_cache_dirname') | ||
|
|
||
| @pytest.mark.parametrize('custom_cache_dirname', [ | ||
| pytest.param('/tmp/mycache', marks=pytest.mark.skipif(sys.platform == 'win32', |
There was a problem hiding this comment.
I think you can use just os.path.abspath('tmp') and it will work for all platforms, no need to parametrize for each.
|
I will do new pull request with support both absolute and relative pathes |
No description provided.