add dest dir for download #27
Conversation
add dest as param for download
There was a problem hiding this comment.
Hello!
Thanks for you contribution.
There are a few things that would need changing before this could be merged.
There are some failing lints that you can find by running npm run eslint. They're also run when you run npm t.
A test (or tests) needs adding for this functionality.
This new functionality needs adding to the usage in https://github.com/tom-james-watson/dat-cp/blob/master/README.md#usage and https://github.com/tom-james-watson/dat-cp/blob/master/src/dcp.js#L21-L29
The prompt asking whether or not to create the dir is nice, but is not necessary. I don't think any other tool like this asks for confirmation for this.
The functionality breaks if you use the --skip-prompt flag. It just downloads to ..
Let me know if you're unsure about any of that.
|
Hi Tom! I made changes requested. Now the prompt doesn't ask the user if he wants to create the directory, he just does it. In the README file I just put the parameter as an option [dest]. Could you check if everything is ok? |
There was a problem hiding this comment.
Hi, thanks for the update and sorry for the slow reply - I was away for much of August and have been busy with other things.
A few things left to clear up, but it's looking good! 💪
One other thing that there's no relevant part of the code to specifically comment on. It would be good if the dry run path list and the file progress paths reflected the out directory. So instead of:
> dcp <key> out --skip-prompt
README.md [========================================] 100% | 0s | 2.53KB
Total: 1 files (2.53KB)
it should be
> dcp <key> out --skip-prompt
out/README.md [========================================] 100% | 0s | 2.53KB
Total: 1 files (2.53KB)
and likewise, instead of
> dcp <key> out --skip-prompt
2.53KB README.md
Total: 1 files (2.53KB)
it should be
> dcp <key> out --skip-prompt
2.53KB out/README.md
Total: 1 files (2.53KB)
| const dat = await Dat({key, sparse: true}) | ||
|
|
||
| if (!options.skipPrompt) { | ||
| const datCpDryRun = new DatCp(dat, {...options, dryRun: true}) | ||
|
|
||
| await datCpDryRun.setDownloadDest(path) |
There was a problem hiding this comment.
This is causing a couple of issues:
- If you do something like
npm run cli -- <key> out -n, you end up with aout/out/file. It only works as expected if you provide the--skip-promptflag. - A dry run is still creating the out directory. A dry run should make no changes to the filesystem.
I don't think this call actually does anything anyway - I would just remove it.
| @@ -135,6 +135,15 @@ export default class DatCp { | |||
| }) | |||
| } | |||
|
|
|||
| async setDownloadDest(path) { | |||
There was a problem hiding this comment.
I think instead of needing to make a separate call to this, there should just be an optional dest property in the options passed to the constructor which causes this method to be called.
An optional function to add dest for download