Repository: theacodes/nox
Pull request #476 (2021-09-12 14:04:52)

Add a "shared cache" directory
Closes #72. Add an `{envdir}/.shared` directory (like @dhermes suggested), and create an API for accesing to it.
I'm a bit new to the concept, so tell me if this is what you want :)

CC @theacodes @cjolowicz

DiddiLeija: Ok. At this point, I have `nox.session.get_shared_cache_dir`, but I don't know at all where should I use it on the sessions.
DiddiLeija: Any suggestions?
theacodes: Looks like a great start. Some suggestions:

* return a `pathlib.Path` object instead of a string.
* make it a `@property` and just call it `cache_dir`. 
DiddiLeija: @theacodes I made your suggestions. Now, I don't know if I have to do something to enable the access to `cache_dir`.
DiddiLeija: Why are the `build` steps on the CI failing?
DiddiLeija: Any suggestions here?
dhermes: @DiddiLeija You are failing test coverage for the newly added code:

```
nox/sessions.py         269      3     86      0    99%   191-193
```

Just need to add tests and cover the new source lines added
theacodes: (It's currently a busy time for me as I'm putting together 500 synth modules to ship out, so I'll get to this soon)
DiddiLeija: No problem, I understand how does it feels :smile:. Review it whenever you want.
FollowTheProcess: Hi @DiddiLeija thanks for the contribution. You're pretty close!

I've checked out your PR locally and have played around with it.

`runner.envdir` is `None` in this test because it hasn't been defined for the test. Notice how the tests around it explicitly set their `runner.envdir` like this example below:

```python
def test_create_tmp_twice(self):
    session, runner = self.make_session_and_runner()
    with tempfile.TemporaryDirectory() as root:
        runner.global_config.envdir = root # Here we explicitly set the envdir
        runner.venv.bin = bin
        session.create_tmp()
        tmpdir = session.create_tmp()
        assert session.env["TMPDIR"] == tmpdir
        assert tmpdir.startswith(root)

```

You should add the following line at the start of `test_properties`:

``` python
def test_properties(self):
    session, runner = self.make_session_and_runner()
    runner.global_config.envdir = ".test" # Set the envdir explicitly, the name doesn't matter

    ...
    # code below
```

I would also make one other change:

Because `session.cache_dir` returns a `pathlib.Path`, this is what we should be testing for in the assertions. So in place of:

```python
# code above
...
assert session.cache_dir == os.path.join(runner.envdir, ".shared")
```

Instead use:

```python
# code above
...
assert session.cache_dir == pathlib.Path(runner.envdir).joinpath(".shared")
```

With these changes, the tests should be passing.
DiddiLeija: Thanks @FollowTheProcess. I will follow your suggestions.
DiddiLeija: I hope this is going to work.
DiddiLeija: I think this PR is ready (at least, there aren't great changes to do).
DiddiLeija: Hmm... it seems like the tests are looking for `.test/test/.shared`. But I think it should be `.test/.shared`, right?
DiddiLeija: @FollowTheProcess Done.
FollowTheProcess: > @FollowTheProcess Done.

Cool, looks like you unindented the `test_properties` function when you did this, so I just fixed that for you and it now looks like this is passing so congrats!

Remember you should always run tests and formatting etc locally first before pushing up 👍🏻 running `nox` would have found this (that's how I found it and why nox is great).
FollowTheProcess: Just want to get another maintainer to take a look at this and make sure they're happy before merging in. cc @theacodes @dhermes 
cjolowicz: Should the directory be named `.cache` for consistency with `session.cache_dir`?

It's a cheap change now, and it would remove a mental translation step (`cache_dir` -> `.shared`) for everybody using this in the future.
FollowTheProcess: > Should the directory be named `.cache` for consistency with `session.cache_dir`?
> 
> It's a cheap change now, and it would remove a mental translation step (`cache_dir` -> `.shared`) for everybody using this in the future.

Yep, makes total sense to me. Agree with your suggestions in the review too.

@DiddiLeija do you want to implement these tweaks or are you happy for us to just fix this up real quick?
DiddiLeija: Of course I can do this simple change ;)
DiddiLeija: Ok. I made your suggestions.

Patch:
diff --git a/nox/sessions.py b/nox/sessions.py
index c97c7eb..5d1b35c 100644
--- a/nox/sessions.py
+++ b/nox/sessions.py
@@ -16,6 +16,7 @@ import argparse
 import enum
 import hashlib
 import os
+import pathlib
 import re
 import sys
 import unicodedata
@@ -184,6 +185,13 @@ class Session:
         self.env["TMPDIR"] = tmpdir
         return tmpdir
 
+    @property
+    def cache_dir(self) -> pathlib.Path:
+        """Create and return a 'shared cache' directory to be used across sessions."""
+        path = pathlib.Path(self._runner.global_config.envdir).joinpath(".cache")
+        path.mkdir(exist_ok=True)
+        return path
+
     @property
     def interactive(self) -> bool:
         """Returns True if Nox is being run in an interactive session or False otherwise."""


Test patch:
diff --git a/tests/test_sessions.py b/tests/test_sessions.py
index f12f02c..3c949e6 100644
--- a/tests/test_sessions.py
+++ b/tests/test_sessions.py
@@ -99,15 +99,20 @@ class TestSession:
 
     def test_properties(self):
         session, runner = self.make_session_and_runner()
+        with tempfile.TemporaryDirectory() as root:
+            runner.global_config.envdir = root
 
-        assert session.name is runner.friendly_name
-        assert session.env is runner.venv.env
-        assert session.posargs == runner.global_config.posargs
-        assert session.virtualenv is runner.venv
-        assert session.bin_paths is runner.venv.bin_paths
-        assert session.bin is runner.venv.bin_paths[0]
-        assert session.python is runner.func.python
-        assert session.invoked_from is runner.global_config.invoked_from
+            assert session.name is runner.friendly_name
+            assert session.env is runner.venv.env
+            assert session.posargs == runner.global_config.posargs
+            assert session.virtualenv is runner.venv
+            assert session.bin_paths is runner.venv.bin_paths
+            assert session.bin is runner.venv.bin_paths[0]
+            assert session.python is runner.func.python
+            assert session.invoked_from is runner.global_config.invoked_from
+            assert session.cache_dir == Path(runner.global_config.envdir).joinpath(
+                ".cache"
+            )
 
     def test_no_bin_paths(self):
         session, runner = self.make_session_and_runner()