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 all Formatters to accept options hash #308

Merged
merged 2 commits into from Dec 29, 2015

Conversation

Projects
None yet
3 participants
@mattbrictson
Copy link
Member

commented Dec 26, 2015

This PR adds optional options to all Formatters via Formatter::Abstract.

None of the built in formatters yet do anything with these options. However, it provides a consistent #initialize API that allows us to promote "formatter options" as a concept further up the stack in Capistrano.

For example, in Capistrano we can offer this configuration system:

set :format, ...
set :format_options, key: value, ...

We can only reasonable offer this if we are sure that the underlying formatters all accept an options hash (i.e. the :format_options).

Most importantly, Airbrussh does have options, and this will allow us to have a standard way for those options to be declared. Airbrussh will be the default formatter in a future version of Capistrano, and this commit is part of laying the groundwork.

@robd

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2015

I think this is good; if I understand, this is just about using DI instead of static lookup of config options from SSHKit.config.

Perhaps this should be a separate PR, but I was wondering if we should use the newly available @options attribute in the Pretty formatter? Or were you thinking the built in SSHKit formatters should carry on using the SSHKit.config API? If we changed this, I think there are tests in unit/formatters which could be simplified to remove some static setup calls to SSHKit.config.output_verbosity.

@mattbrictson

This comment has been minimized.

Copy link
Member Author

commented Dec 26, 2015

this is just about using DI instead of static lookup of config options from SSHKit.config.

I hadn't really thought it through that far, but yes, that makes sense. Right now SSHKit isn't injecting any of its own options, but it could inject output_verbosity from the config, which would relieve the formatter from having to do a static lookup.

Perhaps this should be a separate PR, but I was wondering if we should use the newly available @options attribute in the Pretty formatter?

Yes, I think this is a natural evolution of the options idea. You also mentioned it might allow us to collapse Pretty and SimpleText into a single formatter using options to produce the two types of output. My vote is to put those things in a separate PR.

@robd

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2015

If we're not going to use the options in SSHKit, it might be helpful to have a test which demonstrates a sample formatter using the options or a comment which explains to people looking at the code why there is an options parameter passed. People may not know that they have to check the Airbrussh formatter constructor signature, or that the Formatter API must not be changed. I guess they can look at the history and check this PR, but someone might think it is a left over attribute which can be deleted. Unfortunately I don't have any time at the moment, otherwise I would do a PR to use the options within SSHKit. But maybe a comment for now just to give people a heads up to check the Airbrussh Repo?

@mattbrictson

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2015

To clarify, here is my reason for normalizing the Formatter constructors to all accept options:

My primary concern is the scenario where someone specifies :format_options for Airbrussh, but then later switches to a different :format, like :pretty.

If they forget to also delete :format_options, and Pretty's constructor doesn't allow an options argument, then suddenly the user gets a runtime error (Pretty#initialize expects 1 argument, got 2).

If all formatters have the same constructor signature, then this problem goes away. At worst, a formatter might be sent options it doesn't care about and can ignore.

Also: I agree some documentation and/or tests are needed to clarify this intention. I'll update this PR.

On Dec 27, 2015, at 11:05 AM, Rob Dupuis notifications@github.com wrote:

If we're not going to use the options in SSHKit, it might be helpful to have a test which demonstrates a sample formatter using the options or a comment which explains to people looking at the code why there is an options parameter passed. People may not know that they have to check the Airbrussh formatter constructor signature, or that the Formatter API must not be changed. I guess they can look at the history and check this PR, but someone might think it is a left over attribute which can be deleted. Unfortunately I don't have any time at the moment, otherwise I would do a PR to use the options within SSHKit. But maybe a comment for now just to give people a heads up to check the Airbrussh Repo?


Reply to this email directly or view it on GitHub.

@robd

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2015

Ahh I see - makes total sense

Allow all Formatters to accept options hash
None of the built in formatters yet do anything with these options. However, it
provides a consistent #initialize API that allows us to promote "formatter
options" as a concept further up the stack in Capistrano.

For example, in Capistrano we can offer this configuration system:

set :format, ...
set :format_options, key: value, ...

We can only reasonable offer this if we are sure that the underlying formatters
all accept an options hash (i.e. the :format_options).

Most importantly, Airbrussh *does* have options, and this will allow us to have
a standard way for those options to be declared. Airbrussh will be the default
formatter in a future version of Capistrano, and this commit is part of laying
the groundwork.

@mattbrictson mattbrictson force-pushed the mattbrictson:formatter-options branch from 0dc79dc to fa7558a Dec 29, 2015

@mattbrictson mattbrictson force-pushed the mattbrictson:formatter-options branch from fa7558a to 75fb82d Dec 29, 2015

@mattbrictson

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2015

I've updated the README and added a test to explain the use of formatter options as @robd suggested. I also rebased onto the latest master commit.

Good to merge?

leehambley added a commit that referenced this pull request Dec 29, 2015

Merge pull request #308 from mattbrictson/formatter-options
Allow all Formatters to accept options hash

@leehambley leehambley merged commit fb37ebc into capistrano:master Dec 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@leehambley

This comment has been minimized.

Copy link
Member

commented Dec 29, 2015

Looks good. Well played, always nice seeing docs in a PR :)

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 17, 2016

taca
Update ruby-sshkit to 1.11.3.
## [1.11.3][] (2016-09-16)

  * Fix known_hosts caching to match on the entire hostlist
    [PR #364](capistrano/sshkit#364) @byroot

## [1.11.2][] (2016-07-29)

### Bug fixes

  * Fixed a crash occurring when `Host@keys` was set to a non-Enumerable.
    @xavierholt [PR #360](capistrano/sshkit#360)

## [1.11.1][] (2016-06-17)

### Bug fixes

  * Fixed a regression in 1.11.0 that would cause
    `ArgumentError: invalid option(s): known_hosts` in some older versions of
    net-ssh. @byroot [#357](capistrano/sshkit#357)

## [1.11.0][] (2016-06-14)

### Bug fixes

  * Fixed colorized output alignment in Logger::Pretty. @xavierholt
    [PR #349](capistrano/sshkit#349)
  * Fixed a bug that prevented nested `with` calls
    [#43](capistrano/sshkit#43)

### Other changes

  * Known hosts lookup optimization is now enabled by default. @byroot

## 1.10.0 (2016-04-22)

  * You can now opt-in to caching of SSH's known_hosts file for a speed boost
    when deploying to a large fleet of servers. Refer to the
    [README](https://github.com/capistrano/sshkit/tree/v1.10.0#known-hosts-caching) for
    details. We plan to turn this on by default in a future version of SSHKit.
    [PR #330](capistrano/sshkit#330) @byroot
  * SSHKit now explicitly closes its pooled SSH connections when Ruby exits;
    this fixes `zlib(finalizer): the stream was freed prematurely` warnings
    [PR #343](capistrano/sshkit#343) @mattbrictson
  * Allow command map entries (`SSHKit::CommandMap#[]`) to be Procs
    [PR #310](capistrano/sshkit#310)
    @mikz

## 1.9.0

**Refer to the 1.9.0.rc1 release notes for a full list of new features, fixes,
and potentially breaking changes since SSHKit 1.8.1.** There are no changes
since 1.9.0.rc1.

## 1.9.0.rc1

### Potentially breaking changes

  * The SSHKit DSL is no longer automatically included when you `require` it.
    **This means you  must now explicitly `include SSHKit::DSL`.**
    See [PR #219](capistrano/sshkit#219) for details.
    @beatrichartz
  * `SSHKit::Backend::Printer#test` now always returns true
    [PR #312](capistrano/sshkit#312) @mikz

### New features

  * `SSHKit::Formatter::Abstract` now accepts an optional Hash of options
    [PR #308](capistrano/sshkit#308) @mattbrictson
  * Add `SSHKit::Backend.current` so that Capistrano plugin authors can refactor
    helper methods and still have easy access to the currently-executing Backend
    without having to use global variables.
  * Add `SSHKit.config.default_runner` options that allows to override default command runner.
    This option also accepts a name of the custom runner class.
  * The ConnectionPool has been rewritten in this release to be more efficient
    and have a cleaner internal API. You can still completely disable the pool
    by setting `SSHKit::Backend::Netssh.pool.idle_timeout = 0`.
    @mattbrictson @byroot [PR #328](capistrano/sshkit#328)

### Bug fixes

  * make sure working directory for commands is properly cleared after `within` blocks
    [PR #307](capistrano/sshkit#307)
    @steved
  * display more accurate string for commands with spaces being output in `Formatter::Pretty`
    [PR #304](capistrano/sshkit#304)
    @steved
    [PR #319](capistrano/sshkit#319) @mattbrictson
  * Fix a race condition experienced in JRuby that could cause multi-server
    deploys to fail. [PR #322](capistrano/sshkit#322)
    @mattbrictson
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.