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

feat(cli): Download path is familiar to users #1039

Merged
merged 1 commit into from Dec 13, 2023

Conversation

iancoleman
Copy link
Contributor

@iancoleman iancoleman commented Dec 4, 2023

Description

Instead of downloading to data_dir which is not a familiar directory to most users, change downloads to be saved to the regular familiar Downloads folder (eg the one most browsers use).

This is especially helpful when cleaning up a disk with low disk space, usually the first place to delete stuff is from the Downloads folder.

Summary generated by Reviewpad on 04 Dec 23 01:42 UTC

This pull request updates the CLI to use a more familiar download path for files. The patch modifies the files_cmds function in the mod.rs file to set the download directory to the system's default download directory. It also updates the download_files function to use the same download directory for file downloads.

@reviewpad reviewpad bot requested a review from jacderida December 4, 2023 01:42
@reviewpad reviewpad bot added Small Pull request is small waiting-for-review labels Dec 4, 2023
@@ -112,7 +112,8 @@ pub(crate) async fn files_cmds(
);
}

let file_api: Files = Files::new(client, root_dir.to_path_buf());
let download_dir = dirs_next::download_dir().expect("Download directory is obtainable");
Copy link
Contributor

@joshuef joshuef Dec 7, 2023

Choose a reason for hiding this comment

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

Can we rework this to avoid a potential panic with the expect here?

That appears to be the issue on the CI runs: https://github.com/maidsafe/safe_network/actions/runs/7102691487/job/19333646110?pr=1039 and https://github.com/maidsafe/safe_network/actions/runs/7102691508/job/19333645696?pr=1039

(Although it's not clear to me why this is failing to find the dir 🤔 , we'd still want to avoid a potential panic in the prod code eitherway)

@joshuef
Copy link
Contributor

joshuef commented Dec 7, 2023

thanks for this @iancoleman 🙇

We'll need to remove the potential for a panic as tehre is now, and also the Memcheck test will need to be updated. (if you search for $CLIENT_DATA_PATH/downloaded_files that'll need to be switched up to ~/Downloads I expect)

@iancoleman iancoleman force-pushed the download_location branch 5 times, most recently from f25e801 to 4f780f7 Compare December 13, 2023 02:56
@iancoleman
Copy link
Contributor Author

@joshuef thanks for the feedback. I've incorporated those changes and I think this is good to go now.

@joshuef joshuef added this pull request to the merge queue Dec 13, 2023
Merged via the queue into maidsafe:main with commit 7a29f07 Dec 13, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Small Pull request is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants