filer icon indicating copy to clipboard operation
filer copied to clipboard

Published 20 hours ago • verified publisher icon filerjs

Issue #122 - Added sh.mv with tests and documentation

Open kwkofler opened this issue 10 years ago • 7 comments

New request -_- Sorry about the last one.

kwkofler avatar Mar 30 '14 03:03 kwkofler

This is a pretty good patch!

@modeswitch Is this approach to mv what we want? I'm fairly sure fs.rename can do the majority of the work for us though I'm still investigating. I need this for #273 , so if rename isn't enough I'll probably update this patch instead of the one I'm developing.

sedge avatar Jan 14 '15 15:01 sedge

I had a quick look at it seems reasonable. The code probably needs to be updated to bring it in line with recent refactoring.

Can you give it a review (adding comments to the patch) and we can discuss any changes you feel are required. If most of the work is done already, let's use this.

modeswitch avatar Jan 14 '15 21:01 modeswitch

@modeswitch The mv utility can accept multiple source files to be moved to a directory. Do we want to support this?

sedge avatar Jan 15 '15 17:01 sedge

The new version of this PR is #346

sedge avatar Jan 15 '15 17:01 sedge

The POSIX standard says that the mv utility relies on rename(), except for cross-filesystem moves. At the moment, this patch is doing the rename() work manually, but it's working. Should I switch it to use fs.rename()?

sedge avatar Jan 15 '15 18:01 sedge

This can't be automerged. Can you investigate?

modeswitch avatar Feb 05 '15 23:02 modeswitch

@modeswitch Here's the updated PR: https://github.com/filerjs/filer/pull/346

It's missing a test case, which shouldn't be too difficult to add. Other than that I believe it's ready for review.

sedge avatar Feb 09 '15 19:02 sedge