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

Allow Rsync::exclude to accept a list of patterns to exclude. #243

Merged
merged 3 commits into from
Feb 5, 2016

Conversation

greg-1-anderson
Copy link
Member

This PR allows taskRsync::exclude() to take an array of patterns, in addition to its traditional parameter value of a single string.

Motivation: sometimes, you might want to call taskRsync with a variable list of locations to exclude.

Before this PR:

    $rsync = $this->taskRsync()
      ->fromPath("$tmpDir/drupal-8/")
      ->toPath($webroot)
      ->args('-a', '-v', '-z')
      ->args('--delete');
    foreach ($excludes as $exclude) {
      $rsync->exclude($exclude);
    }
    $rsync->run();

After this PR:

    $this->taskRsync()
      ->fromPath("$tmpDir/drupal-8/")
      ->toPath($webroot)
      ->args('-a', '-v', '-z')
      ->args('--delete')
      ->exclude($excludes)
      ->run();

return $this->option('exclude', escapeshellarg($pattern));
if (is_array($pattern)) {
foreach ($pattern as $item) {
$this->option('exclude', escapeshellarg($item));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call $this->exclude here to avoid duplication.

@boedah
Copy link
Contributor

boedah commented Dec 1, 2015

Useful PR!
Please see my comment.

@greg-1-anderson
Copy link
Member Author

Sure thing! Thank you for the feedback.

@boedah
Copy link
Contributor

boedah commented Dec 2, 2015

👍

…ionList. Add 'includeFilter' and 'filter' rsync options.
@greg-1-anderson
Copy link
Member Author

At the risk of complicating this issue, I pushed another commit for consideration, adding 'includeFilter' and 'filter' options as well.

To avoid repeating the same pattern over and over again, I factored the option list implementation into an optionLis() method in CommandArguments. The complication here is that, in order to be useful as a shorthand method, optionList() must call escapeshellarg on the values passed to it. This, however, makes optionList() inconsistent with option(), which does not escape.

Changing option() is not a good option (no pun intend), because existing code expects that escapeshellarg is necessary, and we do not want to double-escape. Forcing callers of optionList to use an awkward array_map to escape their array values would be possible, but this just seemed awkward.

In the end, I merely documented the difference, but could do it the other way if desired.

@greg-1-anderson
Copy link
Member Author

Bump. This would be good to get in, but please consider the consistency-vs-backwards-compatibility issue above.

@DavertMik
Copy link
Member

Ok, looks good.

DavertMik added a commit that referenced this pull request Feb 5, 2016
Allow Rsync::exclude to accept a list of patterns to exclude.
@DavertMik DavertMik merged commit 5200ff5 into consolidation:master Feb 5, 2016
@greg-1-anderson greg-1-anderson deleted the exclude-list branch February 5, 2016 04:14
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.

None yet

3 participants