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

Change cache directory structure to include pytest #3150

Merged
merged 2 commits into from
Jan 27, 2018

Conversation

alanbato
Copy link
Member

Hi all. This should deal with the issue discussed in #3138. I've basically done the following:

  • Change the v folder to pytest_values
  • Change the cache/lastfailed directory/file to just lastfailed inside the pytest_values directory
  • Change the tests accordingly.

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS, in alphabetical order;

@coveralls
Copy link

coveralls commented Jan 25, 2018

Coverage Status

Coverage remained the same at 92.626% when pulling cbbd606 on alanbato:pytest_cache into b8be339 on pytest-dev:features.

@nicoddemus
Copy link
Member

Hi @alanbato thanks for the PR!

There are a couple of issues in my POV though:

  1. Currently the root directory is .cache; I believe this is the heart of Include the name "pytest" somewhere in the cache directory name #3138, that there's no indication that this directory has anything to do with pytest; I believe we should try to add the pytest name in there, perhaps just renaming it to .pytest_cache.
  2. The internal structure should not really change. The keys defined by the users of the "cache" API should always include some kind of application name (cache/lastfailed, myapp/some_info, etc) to avoid naming clashes.

In summary I think we should just change the root directory from .cache to .pytest_cache. 😁

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.

See comment

@alanbato
Copy link
Member Author

Oh, I totally misunderstood what was required then 😅
I'll revert back my changes and work on your idea right now :)

@nicoddemus
Copy link
Member

@alanbato the description could be clearer actually, my apologies.

Thanks for working on this. 👍

@alanbato
Copy link
Member Author

My pleasure. Hopefully I can work on some other minor issues and bugs and go up from there :)

By the way @nicoddemus, just to be sure, this should be regarded as a 'feature', right? Or is another label more appropiate? (For the changelog and such)

@nicoddemus
Copy link
Member

My pleasure. Hopefully I can work on some other minor issues and bugs and go up from there :)

That's awesome! Please don't hesitate to ask for help if you run into any roadblocks.

By the way @nicoddemus, just to be sure, this should be regarded as a 'feature', right? Or is another label more appropiate? (For the changelog and such)

Yeah it is a "feature" in the sense that it is a new behavior which is not an actual bug-fix. 👍

@nicoddemus
Copy link
Member

Thanks @alanbato, LGTM now.

We should expect some complaints because people will need to update their .gitignore files, but other than that I don't expect too much disruption.

@RonnyPfannschmidt any thoughts?

@The-Compiler
Copy link
Member

What about old .cache/v/cache/lastfailed files (and potentially plugins putting their own stuff there)? Shouldn't we ideally move things over? That comes with quite a few corner cases though, and for lastfailed it's not critical (but it's still old files rotting) - however, for other information it might be?

@RonnyPfannschmidt
Copy link
Member

@The-Compiler since its a "cache" i consider a migration tool unnecessary, however we might want to support the old folder of it exists

@nicoddemus
Copy link
Member

since its a "cache" i consider a migration tool unnecessary

I agree, critical information should not be stored there, and a migration tool seems unnecessary after all a simple mv .cache .pytest_cache is enough (we might want to document that). 😁

however we might want to support the old folder of it exists

My initial take on this looked hard to implement, but what if we just move .cache to .pytest_cache during the plugin initialization temporarily while in 3.4? We can then remove this code in 3.5 or later when we expect most users to already have the cache folder under the new name.

@RonnyPfannschmidt
Copy link
Member

well - the current state is mergeable from my pov, lets have the sorting of compatibility for afterwards, as its not critical

@nicoddemus
Copy link
Member

Thanks again @alanbato for the contribution! 👍

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.

5 participants