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

Add support for moving directories #260

Closed
coxp opened this issue Jun 2, 2015 · 6 comments
Closed

Add support for moving directories #260

coxp opened this issue Jun 2, 2015 · 6 comments
Labels
Milestone

Comments

@coxp
Copy link

coxp commented Jun 2, 2015

I think the API should follow that for moving files.

  • MoveDirectoryToDirectory(directoryPath, targetDirectoryPath) - Moves an existing directory to a new location (e.g. Move "build/project" to "package" results in "build/" and "package/project"). Target location must exist but full path to target location (once combined with directoryPath) must not.
  • MoveDirectories(pattern, targetDirectoryPath) - Moves existing directories matching the specified patern to a new location. Actual move implementation as MoveDirectoryToDirectory.
  • MoveDirectories(directoryPaths, targetDirectoryPath) - Moves existing directories to a new location. Actual move implementation as MoveDirectoryToDirectory.
  • MoveDirectory(directoryPath, targetDirectoryPath) - Moves an existing directory to a new location, providing the option to specify a new directory name. Target location must not exist. (e.g. Move "build/project" to "package" results in "build/" and "package/" if "package" did not already exist and an error if it did, whereas move "build/project" to "package/project" results in "build/" and "package/project").

I think the MoveDirectory API should only work for directories on the same volume. Alternatively, if we determine the targetDirectoryPath is on a different volume (not sure if there is an existing method for this?) then we could implement a method to create a new directory on the the target volume, move the files within that directory, recursively apply for any sub-directories, and then delete the source directory.

@RichiCoder1
Copy link
Contributor

I could be reading this wrong, but I have some objections to the MoveDirectoryToDirectory and the suggested MoveDirectory semantics. Move directory should have a behavior as close to possible to existing "move" commands on systems as possible. There should be a single MoveDirectory alias, relative directories should be based of the CWD, there should be no cross volume restriction, and the implemenation should defer to .NET or native APIs for actually moving the directory. Overwrite should be an explict option you have to set, and Move-ing a directory in such a way that it would conflict should throw an exception unless that option is specifically set to true. I don't know how I feel about MoveDirectory creating any missing parent directories, but would not be a bad idea.

MoveDirectories worries me, specifically MoveDirectories(pattern, targetDirectoryPath).

@coxp
Copy link
Author

coxp commented Jun 2, 2015

The cross volume restriction for moving a directory is from the underlying .NET API for Directory.Move. In addition, you can't overwrite an existing directory.

Personally, I like the symmetry with the Cake File aliases such as MoveFileToDirectory and MoveFiles and I'm glad the Cake APIs don't follow the underlying .NET API. I also think it's simpler for a build system where the main (only?) reason to move directories is for packaging. I want to move a bunch of directories into a packaging directory. I don't think we should support overwriting a directory at all though.

I'm happy to go with any API though. For my purposes it would just mean having to duplicate the leaf folder in the source and destination DirectoryPaths.

MoveDirectory(projectDir + Directory("settings"), packagingDir + Directory("settings")

@daveaglick
Copy link
Member

Going to submit a PR for this shortly. I'm only going to focus on the single MoveDirectory(DirectoryPath, DirectoryPath) alias. I agree with @RichiCoder1 that the other aliases performing multiple directory operations are troublesome.

As with File.Move(FilePath), I'll create a Directory.Move(DirectoryPath) method to do the work that primarily defers to DirectoryInfo.MoveTo(string) so we'll inherit it's restrictions and semantics including the same volume restriction and the restriction on moving one directory on top of another.

@coxp
Copy link
Author

coxp commented Oct 19, 2016

Nice one, this will knock at least a minute of my current build time.

Not sure what why moving multiple directories is any more troublesome than moving multiple files but maybe I'm missing something? Also, I think it's a shame that the Cake File APIs and Directory APIs will not be consistent (the file APIs do not behave the same as the underlying .NET file APIs), but it's hardly the end of the world.

@patriksvensson
Copy link
Member

@coxp Out of interest, where do the file APIs not behave the same as the underlying .NET file APIs?

@devlead
Copy link
Member

devlead commented Oct 20, 2016

Fixed by #1302

@devlead devlead closed this as completed Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants