Added keyword argument, default_opts, to rsync_project. #910

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@moorepants
Contributor

Previously there was no way to remove any of the predefined rsync options. This
change allows them to be modified via a keyword argument.

This does seem to make the extra_opts argument slightly redundant. I could remove it, but there would be backwards incompatibilities.

I also don't understand why the full call signature (all args) doesn't render in the Sphinx docs for rsync_project. The docs and usage would be clearer if the call signature didn't revert to:

fabric.contrib.project.rsync_project(_args, *_kwargs)

Instead rendering as:

fabric.contrib.project.rsync_project(remote_dir, local_dir=None, exclude=(), delete=False,
default_opts='-pthrvz', extra_opts='', ssh_opts='', capture=False,
upload=True)

@bitprophet
Member

@moorepants The Sphinx question is easy: Sphinx is kinda broken about this crap and requires one to manually write out (and then...remember to update...which never happens. Humans! Including myself!) the signature in the Sphinx doc stub. I believe this remains unsolved without wacky decorator-circumventing hijinx.

Re: the patch, I approve of the idea, though yes we can't really nuke extra_opts in the 1.x series for backwards compat. Some small nitpicks:

  • This is a new feature of sorts, not a bugfix, so I'd prefer it were based off master and the versionchanged/etc stuff set to refer to '1.7'. I promise not to let it sit around for another month (sigh).
  • Please move the new kwarg to the end of the kwarg list - also for backwards API compat. Sadly, you never know when folks have been relying on positional argument order :(
  • Please credit yourself in the changelog entry, unless you really don't want to :)
moorepants added some commits Jun 5, 2013
@moorepants @moorepants moorepants Added keyword argument, default_opts, to rsync_project.
Previously there was no way to remove any of the predefined rsync options. This
change allows them to be modified via a keyword argument.
429a638
@moorepants moorepants Rebased off 1.7.0 and moved arg order. 9343f5b
@moorepants
Contributor

@bitprophet Is this good?

@bitprophet
Member

I promise not to let it sit around for another month (sigh).

HEAD

DESK

:(

I'll poke at this and merge it ASAP (& will manually pave over any remaining merge issues/tweaks to avoid more ticket tennis). This should also solve/obviate/replace #967 and #883 at the very least. Thanks!

@bitprophet
Member

Applied editorial tweaks, then spent forever dicking around with the fledgling integration test suite so I could actually test the changes at a basic level. Seem to have succeeded, waiting for Travis-CI to confirm I am not insane before I merge :)

EDIT: Except they're either backed up or something's preventing Travis from seeing my integration branch. Meh. All passes locally; merged and pushed master.

@bitprophet bitprophet added a commit that referenced this pull request Sep 5, 2013
@bitprophet bitprophet Integration tests re: #910.
Turns out the mock_streams stuff wasn't strictly necessary.
Keeping it in anyway since it will be later.
b3707af
@bitprophet
Member

Tests pass now; closing. I rebased at one point & github doesn't play well there. This will come out in Friday's feature release, 1.8.

@bitprophet bitprophet closed this Sep 5, 2013
@moorepants
Contributor

Cool thanks for getting this in!

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