feat(): Add copy implementation #1758
Conversation
|
Looks good to me! Thanks @27pchrisl for adding this! |
There was a problem hiding this comment.
Is working fine for web platform, but there are some problems in the other platforms, I've commented the problems so you can fix them.
Also, the example app are not tests that automatically run, but something to manually test, so I think having all that functions in a single button can be overkill, can you add a few more buttons and split the functions? at least one for copy functions and another for rename functions.
Other than that, awesome PR!
| return | ||
| } | ||
|
|
||
| let directoryOption = call.get("directory", String.self, DEFAULT_DIRECTORY)! | ||
| var toDirectoryOption = call.get("toDirectory", String.self, DEFAULT_DIRECTORY)! |
There was a problem hiding this comment.
| var toDirectoryOption = call.get("toDirectory", String.self, DEFAULT_DIRECTORY)! | |
| var toDirectoryOption = call.get("toDirectory", String.self, "")! |
Set the default to "", otherwise the following if will never be true and will always copy to DOCUMENTS directory as it's the default if no "toDirectory" is provided
electron/src/electron/filesystem.ts
Outdated
| }); | ||
| } | ||
|
|
||
| copy(options: CopyOptions): Promise<CopyResult> { | ||
| return this._copy(options, true); |
There was a problem hiding this comment.
| return this._copy(options, true); | |
| return this._copy(options, false); |
copy doesn't rename, so should be false
There was a problem hiding this comment.
Good catch, missed this during refactoring
electron/src/electron/filesystem.ts
Outdated
| } | ||
|
|
||
| rename(options: RenameOptions): Promise<RenameResult> { | ||
| return this._copy(options, false); |
There was a problem hiding this comment.
| return this._copy(options, false); | |
| return this._copy(options, true); |
Here the opposite, rename should be true on rename function
| @@ -455,28 +493,72 @@ public void rename(PluginCall call) { | |||
| } | |||
There was a problem hiding this comment.
this is preventing from copying/renaming if the path is the same but the directory is different, you should compare fromObject and toObject instead.
…ory as it's the default if no "toDirectory" is provided
|
I've also hugely simplified the tests into four buttons, and removed all the negative tests and kept the main gist of it. |
Implementation for all platforms, and uses the same copy semantics across all.
This method is called 'copy' (rather than node-style copyFile), since it can copy both directory structures and individual files.
It turns out that a copy operation has almost everything in common with a rename operation, so in each case there is a _copy method that does the bulk of the param checking/error handling etc. so as not to end up with a lot of duplication.
Copy, and now rename support a 'toDirectory' as well as the pre-existing 'directory'. Developers can omit 'directory' to use DEFAULT_DIRECTORY, include only 'directory' to rename/copy within a directory, or include both to rename/copy between directories.
Finally, added a set of tests to the example app to show the move/copy operations working.