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

Introduce DelegatingFormatter #36

Closed
wants to merge 3 commits into from
Closed

Conversation

mattbrictson
Copy link
Owner

This PR contains the first round of refactoring as discussed in #24.

Specifically, this adds DelegatingFormatter, which forwards logging methods to 1 or more other formatters. This will allow us to split out the responsibilities of console logging and file (pretty) logging into two separate formatter classes.

I have not yet done this split, so this PR is just laying the groundwork.

@robd I'd be interested in hearing your feedback if you have time. I would like to make sure this is going in the right direction before doing further refactoring.

One interesting consequence of this PR is that SSHKit::Formatter::Airbrussh is now more than just an alias for Airbrussh::Formatter: the former now potentially delegates to multiple formatters. As a result, users of Airbrussh will need to use SSHKit::Formatter::Airbrussh if they want the correct behavior. So this is an API break. (Although we are pre-1.0, so I guess that doesn't really matter.)

The purpose of this class is to forward logging methods to 1 or more other
formatters.
This makes SSHKit::Formatter::Airbrussh a subclass of
Airbrussh::DelegatingFormatter, which will facilitate future refactoring.
The Configuration object now gains the responsibility of being a factory for
the formatter objects that will be delegated to.
@robd
Copy link
Contributor

robd commented Jul 6, 2015

I think this looks like a good starting point. It's a bit hard to predict exactly whether this code will be the right shape until we can see how the formatters split out, but off the top of my head it seems sane and in line with what we discussed.

The following is a matter of personal preference, but perhaps might be worth holding off merging this until the follow up PR (or additional commits to this PR) to actually do the split is done and we are sure the code is in the right shape. Especially since it involves a breaking change to the API.

On the subject of the naming change, I'm not sure if I fully understand this. But would a solution to this to be to rename the current Airbrussh::Formatter, for example to Airbrussh::ConsoleFormatter, and rework Airbrussh::Formatter to be the formatter which extends DelegateFormatter and sets up the delegates.

Seeing it in code, I also wonder if it is premature to introduce DelegateFormatter at this stage. I might be inclined to combine this with the subclass; I think having what is essentially a no-op subclass makes it harder to understand. I'm thinking perhaps, something like this:

module Airbrussh
  class Formatter
    FORWARDED_METHODS = %w(
      fatal error warn info debug log
      log_command_start log_command_data log_command_exit
      << write
    )

    attr_reader :formatters

    def initialize(io, config=::Airbrussh.configuration)
      super(config.formatters(io))
    end

    FORWARDED_METHODS.each do |method|
      define_method(method) do |*args|
        formatters.map { |f| f.public_send(method, *args) }.last
      end
    end
  end
end

module Airbrussh
  class ConsoleFormatter
  # Code which is currently in Airbrussh::Formatter
  end
end

# SSHKit::Formatter::Airbrussh goes back to being an alias for Airbrussh::Formatter
module SSHKit
  module Formatter
    class Airbrussh < Airbrussh::Formatter
    end
  end
end

module Airbrussh
  class Configuration
    # ...
    def formatters(io)
      [Airbrussh::ConsoleFormatter.new(io, self)]
    end
  end
end

Apologies if I haven't fully understood the relationship between the classes, hopefully this is of some help.

@mattbrictson
Copy link
Owner Author

Good idea on the rename to ConsoleFormatter in order to leave the Airbrussh::Formatter as the main entry point. I'll work on that next, plus add in the log file formatter (which will mean the delegate becomes more than just a no-op).

Thanks!

@robd
Copy link
Contributor

robd commented Jul 6, 2015

OK great! BTW, I'm not sure if ConsoleFormatter is the best name for it; I was trying to come up with a name which contrasted with PrettyLogFileFormatter and I couldn't think of anything else.

@mattbrictson
Copy link
Owner Author

Closed in favor of #38.

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.

None yet

2 participants