Skip to content

Conversation

@Caid11
Copy link
Contributor

@Caid11 Caid11 commented Oct 24, 2015

Added support for recursive copying to copy_file. Now, when a directory path is supplied to local_file, the method recursively calls itself on every file and directory within that directory, and attempts to copy them to the directory path supplied to remote_file.

Copy link
Member

Choose a reason for hiding this comment

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

What if local_file is a directory?

Nevermind, can see the other code path.

@pkittenis
Copy link
Member

Thanks for the PR!

Am making changes to copy_file to fix #41 - will have to rebase on that once they're finished.

This is the working branch for that WIP.

Couple comments in the meantime:

  • There should be a flag to turn recurse on/off - off by default (current behaviour)
  • copy_file would best be recursive if wanting to keep the functionality to one function. The call to copy_file here is not recursive and can exhaust stack space with a sufficiently large directory tree. In addition, to keep copy_file behaviour simple (branching code paths for dir/file copy), best to make recursive dir copy into new function that calls copy_file and not have copy_file call itself.

Eg make new function copy_dir, at copy_file do if recurse and os.path.isdir(local_file): copy_dir(<..>) and in copy_dir call copy_file per file copy.

  • Changes don't seem to handle local_file being a directory - see comment above. - They do.

If you want to rebase now, can assume that mkdir will recursively create sub-directories that don't exist - that will be covered as part of #41 .

@Caid11
Copy link
Contributor Author

Caid11 commented Oct 27, 2015

Thanks for the feedback! I'll make those changes and update the PR.

@Caid11
Copy link
Contributor Author

Caid11 commented Oct 28, 2015

Rebased onto mkdir_fix branch, broke logic for copying directories into new _copy_dir function, and added a recurse flag that has to explicitly be set to True for the function to descend into directories.

…Added unittests for mkdir for relative path, absolute paths and multiple missing parent dirs
Copy link
Member

Choose a reason for hiding this comment

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

The new recurse keyword and functionality needs documenting here.

@Caid11
Copy link
Contributor Author

Caid11 commented Oct 28, 2015

I think that's a great way to do it. It makes it more clear what the recurse flag is doing for anyone reading the code, and makes the code a bit cleaner to look at.

I'll update the documentation and make that change. Thanks again for the feedback!

@pkittenis
Copy link
Member

Awesome, thanks!

Have merged the mkdir_fix branch into master so should be able to rebase on master branch now (it's showing some conflicts here).

Kincaid Savoie added 2 commits October 28, 2015 18:32
… into sftp_recursive

Conflicts:
	pssh/ssh_client.py

Merged in new mkdir function.
@Caid11
Copy link
Contributor Author

Caid11 commented Oct 29, 2015

Rebased sftp_recursive onto parallel-ssh/master.

Sorry for the messy commit history; this is the first time I've used git to do more than just push and pull.

@pkittenis pkittenis closed this Oct 29, 2015
@pkittenis
Copy link
Member

Looks like a bad rebase + merge has created bad history - PR changes are showing changes from mkdir_fix branch.

Have re-created the PR at #45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants