cache clear: make cache name optional to clear all caches #10627
+84
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAllow Sequence diagram for the updated cache clear command behaviorsequenceDiagram
actor User
participant CLI as Poetry_CLI
participant Cmd as CacheClearCommand
participant Cfg as Config
participant FC as FileCache
participant FS as FileSystem
User->>CLI: poetry cache clear --all
CLI->>Cmd: invoke handle()
Cmd->>Cmd: cache = argument(cache) (None)
alt cache not provided
Cmd->>Cmd: parts = []
Cmd->>Cmd: root = ""
else cache provided
Cmd->>Cmd: parts = cache.split(":")
Cmd->>Cmd: root = parts[0]
end
Cmd->>Cfg: Config.create()
Cfg-->>Cmd: config
Cmd->>Cmd: cache_dir = config.repository_cache_directory / root
Cmd->>FS: check cache_dir.parent exists
FS-->>Cmd: exists or not
alt cache_dir.parent missing
Cmd->>FS: create cache_dir.parent
end
Cmd->>FC: FileCache(cache_dir)
alt len(parts) < 2
Cmd->>Cmd: check option(all)
alt option all is False
Cmd->>User: RuntimeError("Add the --all option if you want to clear all caches")
else option all is True
Cmd->>FS: check cache_dir exists
FS-->>Cmd: exists or not
alt cache_dir does not exist
alt root is empty
Cmd->>User: line("No cache entries")
else root not empty
Cmd->>User: line(f"No cache entries for {root}")
end
Cmd-->>CLI: return 0
else cache_dir exists
Cmd->>FS: iterate and delete cache entries
Cmd->>User: line("Cache entries cleared")
Cmd-->>CLI: return 0
end
end
else len(parts) >= 2
Cmd->>FC: clear subset of cache based on parts
Cmd->>User: line("Selected cache entries cleared")
Cmd-->>CLI: return 0
end
Updated class diagram for cache clear command and related classesclassDiagram
class Command {
+str name
+str description
+list arguments
+list options
+int handle()
}
class CacheClearCommand {
+str name
+str description
+list arguments
+list options
+int handle()
}
class Argument {
+str name
+str description
+bool optional
}
class Option {
+str name
+str description
}
class Config {
+Path repository_cache_directory
+Config create()
}
class FileCache {
+Path cache_dir
+FileCache(Path cache_dir)
+int __len__()
+void clear_all()
+void clear_subset(str root, list parts)
}
Command <|-- CacheClearCommand
CacheClearCommand --> Argument : uses
CacheClearCommand --> Option : uses
CacheClearCommand --> Config : uses
CacheClearCommand --> FileCache : uses
CacheClearCommand : arguments = [Argument(cache, optional=True)]
CacheClearCommand : options = [Option(all)]
CacheClearCommand : handle()
CacheClearCommand : - str cache
CacheClearCommand : - list parts
CacheClearCommand : - str root
Argument : Argument(str name, str description, bool optional)
Option : Option(str name, str description)
Config : create()
FileCache : FileCache(Path cache_dir)
FileCache : clear_all()
FileCache : clear_subset(str root, list parts)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The control flow in handle() could be clearer if you handled the "no cache argument" case early and separately (e.g., early return for
cache is None), instead of relying onroot = ""andparts = []and reusing the samelen(parts) < 2logic for both cases. - The error message raised when a specific cache is provided without
--allwas changed to "Add the --all option if you want to clear all caches", which is a bit misleading in that context; consider preserving the cache name in the message to keep the guidance specific (as it was before).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The control flow in handle() could be clearer if you handled the "no cache argument" case early and separately (e.g., early return for `cache is None`), instead of relying on `root = ""` and `parts = []` and reusing the same `len(parts) < 2` logic for both cases.
- The error message raised when a specific cache is provided without `--all` was changed to "Add the --all option if you want to clear all caches", which is a bit misleading in that context; consider preserving the cache name in the message to keep the guidance specific (as it was before).
## Individual Comments
### Comment 1
<location> `tests/console/commands/cache/test_clear.py:38` </location>
<code_context>
+def test_cache_clear_all(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for `cache clear` without a cache name and without `--all` to cover the new error path.
This new behavior now triggers the branch where `len(parts) < 2` and `--all` is not set, raising a `RuntimeError` with the message: "Add the --all option if you want to clear all caches". Please add a test (e.g. `test_cache_clear_without_all_raises`) that runs `cache clear` with no arguments, asserts that it fails, and checks this exact message to lock in the CLI behavior and avoid regressions.
</issue_to_address>
### Comment 2
<location> `tests/console/commands/cache/test_clear.py:59-60` </location>
<code_context>
+ assert caches[1].has("cashy:0.2")
+
+
[email protected]("inputs", ["yes", "no"])
+def test_cache_clear_all_one_cache(
+ tester: ApplicationTester,
+ repository_cache_dir: Path,
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for `cache clear --all` (with and without a cache name) when the cache directory does not exist to validate the "No cache entries" message.
Since the command now prints different messages depending on whether a root is provided, please add tests for both cases when the cache directory does not exist: `cache clear some-repo --all` and `cache clear --all`. Each test should assert a successful exit and that the output message matches the expected text. This will fully verify the updated behavior for non-existent caches.
Suggested implementation:
```python
caches: list[FileCache[dict[str, str]]],
inputs: str,
) -> None:
exit_code = tester.execute(f"cache clear {repositories[0]} --all", inputs=inputs)
def test_cache_clear_all_with_cache_name_no_cache_dir(
tester: ApplicationTester,
repository_cache_dir: Path,
repositories: list[str],
) -> None:
# Remove the cache directory to simulate a non-existent cache for the given root
repository_cache_dir.rmdir()
exit_code = tester.execute(f"cache clear {repositories[0]} --all")
assert exit_code == 0
output = tester.io.fetch_output()
assert "No cache entries" in output
# The message should mention the specific cache root when one is provided
assert repositories[0] in output
def test_cache_clear_all_without_cache_name_no_cache_dir(
tester: ApplicationTester,
repository_cache_dir: Path,
) -> None:
# Remove the cache directory to simulate a non-existent cache when no root is provided
repository_cache_dir.rmdir()
exit_code = tester.execute("cache clear --all")
assert exit_code == 0
output = tester.io.fetch_output()
assert "No cache entries" in output
# The message should clearly indicate the absence of entries without tying it to a specific root
assert "cache" in output or "directory" in output
```
To fully align these tests with your actual CLI messages and test helpers:
1. Verify how `ApplicationTester` exposes command output in your codebase. If it uses a different method than `tester.io.fetch_output()` (e.g. `tester.io.fetch_output().strip()` or `tester.io.fetch_error_output()`), adjust the calls accordingly.
2. Replace the loose assertions on `"cache"` / `"directory"` with checks against the exact expected messages for:
- `cache clear some-repo --all` when the cache directory does not exist
- `cache clear --all` when the cache directory does not exist
For example, if the implementation prints `No cache entries for cache root "some-repo".`, assert that string (using `repositories[0]` where appropriate); and if it prints `No cache entries in cache directory.`, assert that exact text.
3. If `repository_cache_dir` might not be empty when these tests run (e.g. if a fixture pre-populates it), replace `repository_cache_dir.rmdir()` with logic that first clears its contents or uses a temporary, distinct non-existent path to simulate the missing cache directory.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
radoering
force-pushed
the
cache-clear-all
branch
from
3e6e9d7 to
0ac8d89
Compare
People have already used the workaround `poetry cache clear --all .`. Let's have an official (and more obvious option).
radoering
force-pushed
the
cache-clear-all
branch
from
0ac8d89 to
e72d2ee
Compare
|
Deploy preview for website ready! ✅ Preview Built with commit e72d2ee. |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
People have already used the workaround
poetry cache clear --all .orpoetry cache clear --all ""See #10585 (comment).
Let's have an official (and more obvious option):
poetry cache clear --allPull Request Check List
Summary by Sourcery
Allow clearing Poetry caches without specifying a cache name and align messaging with the broader behavior.
New Features:
poetry cache clear --allwithout requiring a cache name.Enhancements:
Documentation:
Tests: