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

Don't strip whitespace from capture() #165

Merged
merged 3 commits into from Sep 10, 2014
Merged

Don't strip whitespace from capture() #165

merged 3 commits into from Sep 10, 2014

Conversation

ghost
Copy link

@ghost ghost commented Jul 25, 2014

Whilst writing some tasks to automate checking up on local submodules, I discovered that the capture() call was stripping leading and trailing whitespace. For some commands that is important information and so it should be preserved.

I also noticed a random failure (occurs around 50% of the time) on one of the tests as follows:

     FAIL (0:00:03.922) test_execute_raises_on_non_zero_exit_status_and_captures_stdout_and_stderr
          --- expected
          +++ actual
          @@ -1,4 +1,4 @@
           "echo exit status: 1
           echo stdout: Nothing written
          -echo stderr: Test capturing stderr
          +echo stderr: Nothing written
           "
        @ test/functional/backends/test_netssh.rb:74:in `test_execute_raises_on_non_zero_exit_status_and_captures_stdout_and_stderr'
          /Users/townsen/.rvm/gems/ruby-2.1.2@sato/gems/mocha-1.1.0/lib/mocha/integration/mini_test/version_2112_to_320.rb:40:in `run'

I observed this test failure even when my capture change was not present, so I ignored it for now.

Lastly, I tidied up the Rakefile a bit so there was a 'test' task that ran both sets of tests, and I used the dependency style of invoking rather than the explicit Rake::Task[].execute as it seems more in the Rake spirit, though if there's a reason for the latter then I'd be happy to hear it.

Nick

@ghost
Copy link
Author

ghost commented Jul 28, 2014

I also made the 'pretty' format somewhat prettier by adding a space between the level and the message. Initially forgot to update the tests but fixed that today.

I'm also using the VMware Fusion vagrant provider so added that to the Vagrantfile

@ghost
Copy link
Author

ghost commented Sep 10, 2014

This PR has been parked for a while - any chance of merging it in? I know that removing the strip may possibly break some existing uses of capture(), but the fix for that (stripping the result manually) is trivial. And capture() should not remove information - even if it is whitespace! It's unfortunate that commands like git submodule status --recursive rely on whitespace, but I don't think that's going to change anytime soon.

leehambley added a commit that referenced this pull request Sep 10, 2014
Don't strip whitespace from capture()
@leehambley leehambley merged commit b3fe36f into capistrano:master Sep 10, 2014
@leehambley
Copy link
Member

All the PRs are parked @townsen I'm struggling on time, but thanks for the bump. I don't disagree with not stripping output, for the most part if a program is badly behaved, people could pipe it to | chomp or sed/awk friends.

@ghost ghost deleted the nostrip_capture branch September 10, 2014 14:11
@ghost
Copy link
Author

ghost commented Sep 10, 2014

Lee, I hear you about time so thanks for that. I'm guilty of not spending time reviewing other PR's - something that might help you out a little. Feel free to ping me on email if you want a second pair of eyes on something - the 'bump' will make a difference. Cheers

@leehambley
Copy link
Member

Usually a "bump" is all I need. My wife is away the next two evenings, so I have some man-time to get some work done :) I'll try and push cap master, and sshkit master out tomorrow, maintaining the changelog/etc and release process is a bit of a nightmare, but I'll get it done asap :)

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