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

Refactor to remove Airbrussh::Colors in favor of SSHKit::Color #63

Closed
mattbrictson opened this issue Oct 27, 2015 · 2 comments
Closed

Comments

@mattbrictson
Copy link
Owner

We have some duplication between Airbrussh and SSHKit in terms of the colorization code. Is there a nice way to consolidate this code for the simplification of both projects?

This is a continuation of the discussion started here: capistrano/capistrano#1509 (comment)

@robd I took a look at this last night, and immediately I ran into some difficulty. Airbrussh and SSHKit approach colorization from opposite directions: Airbrussh applies colors regardless, and then strips the ANSI color escape codes conditionally during output (in Console); SSHKit applies colors conditionally, based on the output of the formatter.

I'm not sure if either approach is necessarily better, but they are at odds. Do you have any ideas of how to approach it?

@robd
Copy link
Contributor

robd commented Oct 27, 2015

I'm not sure - my gut feeling is that it's simpler not to add the colors in the first place if they are not needed, but perhaps this has downsides I haven't thought of.

One difference is that in the SSHKit approach, colors wouldn't be stripped from output of other commands called by SSHKit. I can't work out if this is a benefit or disadvantage. If I understand correctly, Airbrussh's config.color = false option strips the colors from the output of commands run by SSHKit. This could be useful if users want the formatter to do this but I'm not sure if that is a common use case.

The changes I made recently continue to allow SSHKit users to force colors to be on, but do not allow users to force them to be off. Since these haven't been released I'm not sure if there are use cases where users want to be able to force the colors to be off even though their output has a tty associated with it. Personally I would be in favour of adding support for the Airbrussh color config options to SSHKit. This was disussed a bit here: capistrano/sshkit#245, but things have moved on quite a bit, so maybe worth discussing again.

If we did add that new option, then in a future major release I would like to remove this colorize override in the SimpleTextFormatter. This would leave a single method - format_message on the SimpleTextFormatter. I think this could be reimplemented as 2 config options on the Pretty formatter:

  1. Whether to show the command UUID
  2. Whether to show the log level prefix (eg INFO, WARN etc)

The SimpleTextFormatter output is the same as the pretty formatter, but with each of these components omitted.

I realise all of this isn't really helping with the mission of making Airbrussh the default Capistrano formatter, but I think, if it makes sense, it would be good if Airbrussh could be configured by using the same options as SSHKit.

If we wanted to reuse the SSHKit color support, then one approach would be to use the colorize method from the abstract formatter, but without adding support for config.color = :true | :false | :auto, this would be a retrograde step for Airbrussh I guess.

I'm not sure if this helps at all? :(

@mattbrictson
Copy link
Owner Author

I don't think this refactor is necessary. Sharing the Colors implementation between SSHKit and Capistrano further couples the internals of the two gems, with not much benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants