lumoslabs / broadside

Command-line tool for AWS ECS deploys
MIT License
44 stars 8 forks source link

General refactoring #57

Closed matl33t closed 7 years ago

matl33t commented 7 years ago

Move GLI logic to better-named files Decoupled command logic from GLI Moved more functionality to target class Decoupled status command from deploy Added TTY and replaced Rainbow Tableized list_targets and renamed to overview

apurvis commented 7 years ago

i just had a brainwave about the Command class refactor, which I had considered before but didn't do because it seemed like almost all the commands needed a Deploy object, so there wasn't a need for both classes. However since you've successfully refactored status out of deploy, i think the "right" answer is that the Command object can be instantiated with a Deploy object, and then rather than having a ton of stuff that looks like this:

def bash(options)
  EcsDeploy.new(options[:target], options).bash
end

you can just have like

class Command
  def initialize(target, config = {})
    @target = target
    @config = config
  end

  delegate :short, :full, :rollback, to: :deploy

  private

  def deploy
    @deploy ||= EcsDeploy.new(@target, @config)
  end
end

(you could prolly use a Struct instead of a class so you don't have to define the reader and initialize methods but the example is clearer with a class)

that's good "composition over inheritance" architecture, and also means you don't have to do EcsDeploy.new(options[:target], options) over and over.

then Command only uses the Deploy object as necessary, but always has a Target, which makes sense given that some commands can be run with just a Target

matl33t commented 7 years ago

Going to have to disagree with Deploy and Command coupling. Only subcommands of broadside deploy should use the Deploy class - the rest, although currently dependent on Deploy, should be moved out (just not in this PR, in interest of time)

apurvis commented 7 years ago

i think we're thinking more similarly than you heard. Command just has a Target as an instance variable, and eventually you would want Command code to just require a Target and not a Deploy, but in the interim you can delegate to: :deploy.

apurvis commented 7 years ago

i think we're basically in total agreement about how Command should work in the end; i'm just suggesting a halfway point that's reachable without too much work