Skip to content
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

Providing custom cache directory through ini option #2558

Closed
wants to merge 9 commits into from
Closed

Providing custom cache directory through ini option #2558

wants to merge 9 commits into from

Conversation

RockBomber
Copy link
Contributor

Providing custom cache directory name through ini option (issue #2543)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 92.142% when pulling 931823e on RockBomber:features_cache_dir into b62aef3 on pytest-dev:features.

@nicoddemus
Copy link
Member

Awesome, thanks!

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following through with absolute paths. 👍

I would like to add support for environment variables as well, but I will do it myself some time later.

@RockBomber
Copy link
Contributor Author

Would you like to add environment variable like PYTEST_CACHEDIR?
Which behaviour must be implemented? Environment variable must override default ini option, but custom ini option must override environment variable?

@nicoddemus
Copy link
Member

Would you like to add environment variable like PYTEST_CACHEDIR?

I hadn't thought about this specifically, but this might be a good idea. 😁

Which behaviour must be implemented? Environment variable must override default ini option, but custom ini option must override environment variable?

I meant to be able to expand variables in the config option (#1089):

[pytest]
cache_dir=$XDG_CONFIG_DIR

This would also mean that we can announce that we plan to change the default from .cache into $XDG_CONFIG_DIR (if defined) in the future.

Are you interested to tackle this as well?

@RockBomber
Copy link
Contributor Author

I can implement it like:

    parser.addini(
        "cache_dir", default=os.environ.get('PYTEST_CACHEDIR', '.cache'),
        help="cache directory path.")

In my opinion, it's look better than cache_dir=$XDG_CONFIG_DIR, because you don't need to create ini file or add advanced args.

@nicoddemus
Copy link
Member

I see, but the main point that I would like to have is environment variable expansion support in the cache_dir variable.

Note that we can have both: PYTEST_CACHEDIR support and a cache_dir config var which knows how to expand environment variables.

Not sure which would take precedence... my initial gut feeling would be that PYTEST_CACHEDIR takes precedence over pytest.ini. Looking at how PYTEST_ADDOPTS is handled:

# args is sys.argv
args[:] = shlex.split(os.environ.get('PYTEST_ADDOPTS', '')) + args
args[:] = self.getini("addopts") + args

PYTEST_ADDOPTS is added after addopts (so it takes precedence), so I think my gut feeling makes sense.

@RockBomber
Copy link
Contributor Author

In this case it may implemented like:

    @staticmethod
    def cache_dir_from_config(config):
        cache_dir = config.getini("cache_dir")
        if cache_dir.startswith('$'):
            env_cache_dir = os.environ.get(cache_dir)
            if not env_cache_dir:
                raise ValueError("environment variable %s is not setted" % cache_dir)
            cache_dir = env_cache_dir
        if _isabs(_expanduser(cache_dir)):
            return py.path.local(cache_dir, expanduser=True)
        else:
            return config.rootdir.join(cache_dir)

@nicoddemus
Copy link
Member

We can let let os.path.expandvars take care of that:

@staticmethod
def cache_dir_from_config(config):
    cache_dir = config.getini("cache_dir")
    cache_dir = os.path.expanduser(cache_dir)
    cache_dir = os.path.expandvars(cache_dir)
    if os.path.isabs(cache_dir):
        return py.path.local(cache_dir)
    else:
        return config.rootdir.join(cache_dir)

@RockBomber
Copy link
Contributor Author

Looks more clear :)
I can add some tests and update PR

@nicoddemus
Copy link
Member

Looks more clear :)
I can add some tests and update PR

That would be great, thanks! 😁

@nicoddemus
Copy link
Member

(Just noticed there is a "First time contributor" small badge on the PR header, didn't know about this GH feature. Nice! 👍)

assert testdir.tmpdir.join(rel_cache_dir).isdir()

def test_custom_abs_cache_dir(self, testdir):
abs_cache_dir = os.path.abspath(testdir.tmpdir.join('custom_cache_dir').strpath)
Copy link
Member

@nicoddemus nicoddemus Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to use a non-sibling of our tmpdir, please use the tmpdir_factory fixture to create a new temporary directory instead:

def test_custom_abs_cache_dir(self, testdir, tmpdir_factory):
    abs_cache_dir = str(tmpdir_factory.mktemp('custom_cache_dir'))

Better to use a non-sibling directory to avoid this feature breaking in the future by accident.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 91.893% when pulling 35469a7 on RockBomber:features_cache_dir into b62aef3 on pytest-dev:features.

@nicoddemus
Copy link
Member

Oh just remembered that this also needs a new changelog file. 👍

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just needs a changelog file and improve the test for absolute path a bit. 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 91.893% when pulling 649a384 on RockBomber:features_cache_dir into ef62b86 on pytest-dev:features.

@RockBomber
Copy link
Contributor Author

Something goes wrong. I will do new PR.
I was confused that in CONTRIBUTING.rst was wrote about adding a message to CHANGELOG.rst
Is it wrong? Must I add change log into changelog directory?

@RockBomber RockBomber closed this Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants