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

Consoliate command formatting helper methods in CommandFormatter #22

Merged
merged 5 commits into from
Jun 24, 2015

Conversation

mattbrictson
Copy link
Owner

This PR builds upon #21 and introduces the Airbrussh::CommandFormatter class. This is a decorator (using Ruby's SimpleDelegator) that wraps an SSHKit::Command to add various formatting helpers.

This consolidates all the command-specific formatting logic in one place. Since CommandFormatter is just a bunch of simple string manipulations, it is also very easy to test.

Along the way I also added Airbrussh::Colors, which is a very basic implementation of the ANSI color logic we need for Airbrussh, with no external dependencies. This replaces our (undeclared) dependency on the colorize gem.

mattbrictson added a commit that referenced this pull request Jun 24, 2015
Consoliate command formatting helper methods in CommandFormatter
@mattbrictson mattbrictson merged commit a854026 into master Jun 24, 2015
@robd
Copy link
Contributor

robd commented Jun 24, 2015

@mattbrictson This also looks very nice and I think there is stuff to think about here which could be applied in the main SSHKit repo.

Especially like removing the Colorize dependency and decorating Commands to apply the necessary command related coloring and formatting. Looks really good.

I am aware that the SSHKit naming of Formatter is not ideal. I think of them as Loggers, in that they have responsibility both for formatting the output and writing the output to log files / stdout.

One thing I have considered is adding support to SSHKit for multiple formatters. I was thinking this might allow us to remove all the SSHKit Pretty formatting related tests from Airbrussh, and remove the management of the Pretty formatted file Logger. One gotcha is the 'last 20 lines of output' currently relies on Airbrussh having access to the pretty formatter.

Another thing which I don't like is when I removed color codes from the log file it unfortunately made the 'last 20 lines of output' message uncolored. Ideally we would show last 20 lines of output with colors on the terminal, but not write character codes to the log file.

@mattbrictson
Copy link
Owner Author

@robd Thanks for the suggestions. I will open issues for them here (and at SSHKit) to serve as the basis for future discussions and possibly PRs.

@mattbrictson
Copy link
Owner Author

@mattbrictson mattbrictson deleted the command-formatter branch January 2, 2017 23:01
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