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.

Sign up for GitHub

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

Jump to bottom

Remove duplication in clearing command stdout / stderr #235

Merged
merged 10 commits into from
Apr 22, 2015
Merged

Remove duplication in clearing command stdout / stderr #235

merged 10 commits into from
Apr 22, 2015

Conversation

robd
Copy link
Contributor

@robd robd commented Apr 22, 2015

This is really preparatory work for supporting pty handlers - #234

This PR grew from me not understanding why these lines that clear stdout/stderr are needed. These were added to as part of #224 to get round a duplicate messages issue, but I wanted to backfill with a failing test so I don't break it when I start fiddling with that code in #234.

I wrote a test case for the Pretty formatter in 61e03af which fails with duplicate messages if the clearing lines are deleted, I also noticed that there was no test at all for the SimpleText formatter, so I backfilled this in c13b10b. This highlighted that the duplicate messages #224 issue is not fixed for the SimpleText formatter. So I fixed this in cb06ab2.

The remaining commits remove duplication within and between the Pretty and SimpleText formatters.

Let me know what you think - I would welcome feedback on code style and so-on.

robd added 9 commits April 21, 2015 22:45
@robd
Add explicit tests for newline behaviour on Pretty formatter
61e03af
@robd
Fixed expected / actual ordering
60df2d9
@robd
Added command lifecycle logging test for pretty formatter
9a3c388 
Try to protect against duplicate log lines caused by the interaction between the backend, command and formatter
@robd
Added failing test for the SimpleText formatter
c13b10b 
I believe issue capistrano#224 also affects the SimpleText formatter. I’ve added a failing test (test_command_lifecycle_logging) to expose the problem.
@robd
Fix for duplicate messages from the SimpleText formatter (capistrano#224
cb06ab2 
)
@robd
Made TestPretty formatter test asserts consistent with TestSimpleText
e4f6ca2
@robd
Introduced clear_stdout_lines and clear_stderr_lines on Command
cd3cdad 
This commit removes duplication in the SimpleText and Pretty formatters by better encapsulating the logic to split and clear stdout/stderr buffers on the Command object.
@robd
Introduced format_std_stream_line() on Abstract formatter
6bdc945 
Factored out duplicated logic from SimpleText and Pretty formatters for formatting a log line from one of the standard streams (ie stdout/stderr).
@robd
Introduced write_command_message method on PrettyFormatter
c9de10d
@leehambley
Copy link
Member

Looked good to me @robd , but there's failures on travis, could you take a look and then I'd be more than happy to merge.

@robd
Copy link
Contributor Author

robd commented Apr 22, 2015

Sorry @leehambley - my bad, I didn't test on ruby 1.9. Just out of interest - do you have any easy way to run the tests against multiple rubies locally?

@robd
Fixed ruby 1.9 compatibility
af63904 
string.lines returns array in ruby 2, but enumerable in 1.9. Convert the enumarable to array - can remove this when we stop supporting ruby 1.9.
@robd
Copy link
Contributor Author

robd commented Apr 22, 2015

@leehambley I fixed the 1.9 compatibility issues, so hopefully this is good to go.

@leehambley
Copy link
Member

do you have any easy way to run the tests against multiple rubies locally?

I use rbenv, personally - but rvm allows this https://rvm.io/set/tests I've honestly never looked to see if rbenv has something.

@leehambley I fixed the 1.9 compatibility issues, so hopefully this is good to go.

Thanks, merged with my gratitude!

leehambley added a commit that referenced this pull request Apr 22, 2015
@leehambley
Merge pull request #235 from robd/formatter-stdout-stderr-clearing
80f1b5e 
Remove duplication in clearing command stdout / stderr
@leehambley leehambley merged commit 80f1b5e into capistrano:master Apr 22, 2015
@robd
Copy link
Contributor Author

robd commented Apr 22, 2015

@leehambley Sorry build breakage is my fault due to a stupid timing assumption. - I am travelling so have dodgy internet, but will get a fix PR to you ASAP

@leehambley
Copy link
Member

Acknowledged Rob, it also passed locally for me, didn't have time to
investigate the Travis build breakage yet, thanks for the clue.
On 22 Apr 2015 5:50 pm, "Rob Dupuis" [email protected] wrote:

@leehambley https://github.com/leehambley Sorry build breakage is my
fault due to a stupid timing assumption. - I am travelling so have dodgy
internet, but will get a fix PR to you ASAP

—
Reply to this email directly or view it on GitHub
#235 (comment).

@robd
Copy link
Contributor Author

robd commented Apr 22, 2015

Fixed in #236. Sorry about that.

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Dec 14, 2015
Update ruby-net-ssh to 2.10.1.rc2.
cee0ba0 
## 1.8.1

  * Change license to MIT, thanks to all the patient contributors who gave
    their permissions.

## 1.8.0

  * add SSHKit::Backend::ConnectionPool#close_connections
    [PR #285](capistrano/sshkit#285)
    @akm
  * Clean up rubocop lint warnings
    [PR #275](capistrano/sshkit#275)
    @cshaffer
    * Prepend unused parameter names with an underscore
    * Prefer “safe assignment in condition”
    * Disambiguate regexp literals with parens
    * Prefer `sprintf` over `String#%`
    * No longer shadow `caller_line` variable in `DeprecationLogger`
    * Rescue `StandardError` instead of `Exception`
    * Remove useless `private` access modifier in `TestAbstract`
    * Disambiguate block operator with parens
    * Disambiguate between grouped expression and method params
    * Remove assertion in `TestHost#test_assert_hosts_compare_equal` that compares something with itself
  * Export environment variables and execute command in a subshell.
    [PR #273](capistrano/sshkit#273)
    @kuon
  * Introduce `log_command_start`, `log_command_data`, `log_command_exit` methods on `Formatter`
    [PR #257](capistrano/sshkit#257)
    @robd
    * Deprecate `@stdout` and `@stderr` accessors on `Command`
  * Add support for deprecation logging options.
    [README](README.md#deprecation-warnings),
    [PR #258](capistrano/sshkit#258)
    @robd
  * Quote environment variable values.
    [PR #250](capistrano/sshkit#250)
    @Sinjo - Chris Sinjakli
  * Simplified formatter hierarchy.
    [PR #248](capistrano/sshkit#248)
    @robd
    * `SimpleText` formatter now extends `Pretty`, rather than duplicating.
  * Hide ANSI color escape sequences when outputting to a file.
    [README](README.md#output-colors),
    [Issue #245](capistrano/sshkit#245),
    [PR #246](capistrano/sshkit#246)
    @robd
    * Now only color the output if it is associated with a tty,
      or the `SSHKIT_COLOR` environment variable is set.
  * Removed broken support for assigning an `IO` to the `output` config option.
    [Issue #243](capistrano/sshkit#243),
    [PR #244](capistrano/sshkit#244)
    @robd
    * Use `SSHKit.config.output = SSHKit::Formatter::SimpleText.new($stdin)` instead
  * Added support for `:interaction_handler` option on commands.
    [PR #234](capistrano/sshkit#234),
    [PR #242](capistrano/sshkit#242)
    @robd
  * Removed partially supported `TRACE` log level.
    [2aa7890](capistrano/sshkit@2aa7890)
    @robd
  * Add support for the `:strip` option to the `capture` method and strip by default on the `Local` backend.
    [PR #239](capistrano/sshkit#239),
    [PR #249](capistrano/sshkit#249)
    @robd
    * The `Local` backend now strips by default to be consistent with the `Netssh` one.
    * This reverses change [7d15a9a](capistrano/sshkit@7d15a9a) to the `Local` capture API to remove stripping by default.
    * If you require the raw, unstripped output, pass the `strip: false` option: `capture(:ls, strip: false)`
  * Simplified backend hierarchy.
    [PR #235](capistrano/sshkit#235),
    [PR #237](capistrano/sshkit#237)
    @robd
    * Moved duplicate implementations of `make`, `rake`, `test`, `capture`, `background` on to `Abstract` backend.
    * Backend implementations now only need to implement `execute_command`, `upload!` and `download!`
    * Removed `Printer` from backend hierarchy for `Local` and `Netssh` backends (they now just extend `Abstract`)
    * Removed unused `Net::SSH:LogLevelShim`
  * Removed dependency on the `colorize` gem. SSHKit now implements its own ANSI color logic, with no external dependencies. Note that SSHKit now only supports the `:bold` or plain modes. Other modes will be gracefully ignored. [#263](capistrano/sshkit#263)
  * New API for setting the formatter: `use_format`. This differs from `format=` in that it accepts options or arguments that will be passed to the formatter's constructor. The `format=` syntax will be deprecated in a future release. [#295](capistrano/sshkit#295)
  * SSHKit now immediately raises a `NameError` if you try to set a formatter that does not exist. [#295](capistrano/sshkit#295)
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Dec 14, 2015
Update ruby-sshkit to 1.8.1.
e938525 
## 1.8.1

  * Change license to MIT, thanks to all the patient contributors who gave
    their permissions.

## 1.8.0

  * add SSHKit::Backend::ConnectionPool#close_connections
    [PR #285](capistrano/sshkit#285)
    @akm
  * Clean up rubocop lint warnings
    [PR #275](capistrano/sshkit#275)
    @cshaffer
    * Prepend unused parameter names with an underscore
    * Prefer “safe assignment in condition”
    * Disambiguate regexp literals with parens
    * Prefer `sprintf` over `String#%`
    * No longer shadow `caller_line` variable in `DeprecationLogger`
    * Rescue `StandardError` instead of `Exception`
    * Remove useless `private` access modifier in `TestAbstract`
    * Disambiguate block operator with parens
    * Disambiguate between grouped expression and method params
    * Remove assertion in `TestHost#test_assert_hosts_compare_equal` that compares something with itself
  * Export environment variables and execute command in a subshell.
    [PR #273](capistrano/sshkit#273)
    @kuon
  * Introduce `log_command_start`, `log_command_data`, `log_command_exit` methods on `Formatter`
    [PR #257](capistrano/sshkit#257)
    @robd
    * Deprecate `@stdout` and `@stderr` accessors on `Command`
  * Add support for deprecation logging options.
    [README](README.md#deprecation-warnings),
    [PR #258](capistrano/sshkit#258)
    @robd
  * Quote environment variable values.
    [PR #250](capistrano/sshkit#250)
    @Sinjo - Chris Sinjakli
  * Simplified formatter hierarchy.
    [PR #248](capistrano/sshkit#248)
    @robd
    * `SimpleText` formatter now extends `Pretty`, rather than duplicating.
  * Hide ANSI color escape sequences when outputting to a file.
    [README](README.md#output-colors),
    [Issue #245](capistrano/sshkit#245),
    [PR #246](capistrano/sshkit#246)
    @robd
    * Now only color the output if it is associated with a tty,
      or the `SSHKIT_COLOR` environment variable is set.
  * Removed broken support for assigning an `IO` to the `output` config option.
    [Issue #243](capistrano/sshkit#243),
    [PR #244](capistrano/sshkit#244)
    @robd
    * Use `SSHKit.config.output = SSHKit::Formatter::SimpleText.new($stdin)` instead
  * Added support for `:interaction_handler` option on commands.
    [PR #234](capistrano/sshkit#234),
    [PR #242](capistrano/sshkit#242)
    @robd
  * Removed partially supported `TRACE` log level.
    [2aa7890](capistrano/sshkit@2aa7890)
    @robd
  * Add support for the `:strip` option to the `capture` method and strip by default on the `Local` backend.
    [PR #239](capistrano/sshkit#239),
    [PR #249](capistrano/sshkit#249)
    @robd
    * The `Local` backend now strips by default to be consistent with the `Netssh` one.
    * This reverses change [7d15a9a](capistrano/sshkit@7d15a9a) to the `Local` capture API to remove stripping by default.
    * If you require the raw, unstripped output, pass the `strip: false` option: `capture(:ls, strip: false)`
  * Simplified backend hierarchy.
    [PR #235](capistrano/sshkit#235),
    [PR #237](capistrano/sshkit#237)
    @robd
    * Moved duplicate implementations of `make`, `rake`, `test`, `capture`, `background` on to `Abstract` backend.
    * Backend implementations now only need to implement `execute_command`, `upload!` and `download!`
    * Removed `Printer` from backend hierarchy for `Local` and `Netssh` backends (they now just extend `Abstract`)
    * Removed unused `Net::SSH:LogLevelShim`
  * Removed dependency on the `colorize` gem. SSHKit now implements its own ANSI color logic, with no external dependencies. Note that SSHKit now only supports the `:bold` or plain modes. Other modes will be gracefully ignored. [#263](capistrano/sshkit#263)
  * New API for setting the formatter: `use_format`. This differs from `format=` in that it accepts options or arguments that will be passed to the formatter's constructor. The `format=` syntax will be deprecated in a future release. [#295](capistrano/sshkit#295)
  * SSHKit now immediately raises a `NameError` if you try to set a formatter that does not exist. [#295](capistrano/sshkit#295)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robd @leehambley