feat: add config cleanup subcommand #95
Conversation
|
I currently added flags to delete specific folders ( --config, --data, --cache). The original issue only stated to specifically delete data and cache. But as it is about deleting files I thought it might be nice to only allow deletions if specific flags are passed and if non are given to do nothing. |
lferrazzini95
force-pushed
the
feat/add-config-cleanup-subcommand
branch
3 times, most recently
from
4c6c68f to
eab9e0c
Compare
| .filter_map(|entry| entry.ok()) | ||
| .filter(|entry| entry.path() != dir) | ||
| .map(|entry| entry.into_path()) |
There was a problem hiding this comment.
supernit: this feels like it could be collapsed but it reads well
There was a problem hiding this comment.
Hi @brooksmtownsend do I got this correctly that you would rather have something like this?
.filter_map(|entry| entry.ok())
.filter_map(|entry| (entry.path() != dir).then(|| entry.into_path()))
Or even more collapsed? I would be interested in the cleanest way by your opinion! :)
crates/wash/src/cli/config.rs
Outdated
| if !(*config || *cache || *data || *all) { | ||
| return Ok(CommandOutput::error( | ||
| "Nothing to do. Please specify --config, --data, --cache, or --all to clean up.", | ||
| Some(serde_json::json!({ | ||
| "message": "Nothing to do. Please specify --config, --data, --cache, or --all to clean up.", | ||
| "success": true, | ||
| })), | ||
| )); | ||
| } |
There was a problem hiding this comment.
I think we could use the clap ArgGroup capability to put config, data, cache, and all in a single arg group, make the group required, and make multiple flags true. That would prevent us from having to update this if we add more fields in the future!
brooksmtownsend
left a comment
brooksmtownsend
left a comment
There was a problem hiding this comment.
Looks great!! I left only optional comments and happy to merge as-is, just want to give ya a chance to take a look and see if you wanted to implement anything (like the clap ArgGroup)
Happy to take these as followup PRs too
crates/wash/src/cli/config.rs
Outdated
| format!("Successfully deleted {} files", successful_deletions), | ||
| Some(serde_json::json!({ | ||
| "message": format!("{} files deleted successfully.",successful_deletions), |
There was a problem hiding this comment.
| format!("Successfully deleted {} files", successful_deletions), | |
| Some(serde_json::json!({ | |
| "message": format!("{} files deleted successfully.",successful_deletions), | |
| format!("Successfully deleted {successful_deletions} files"), | |
| Some(serde_json::json!({ | |
| "message": format!("{successful_deletions} files deleted successfully."), |
This format better matches the rest of the crate's usage
lferrazzini95
force-pushed
the
feat/add-config-cleanup-subcommand
branch
from
eab9e0c to
c68f19b
Compare
|
Hi @brooksmtownsend I am so sorry for the long delay of my changes had a lot going on... I considered your remarks and hope this is fine now. |
lferrazzini95
force-pushed
the
feat/add-config-cleanup-subcommand
branch
4 times, most recently
from
51b0932 to
778ba5c
Compare
Signed-off-by: Eric Gregory <[email protected]>
lferrazzini95
force-pushed
the
feat/add-config-cleanup-subcommand
branch
from
778ba5c to
a7fbc1e
Compare
3 participants
Feature or Problem
Currently user do not have the ability to easily clean up cache, config or data files related to wash. Therefore a subcomand was introduced to clean up all directories.
Related Issues
Issue 29
Release Information
nextConsumer Impact
None
Testing
Currenlty no tests are added but probably it would be nice to add an integration test to check that the only the correct files are deleted. Waiting on Input on this.
Unit Test(s)
Added unit test to list all objects in a specific directory.
Acceptance or Integration
None
Manual Verification
Verified manually