Yes, it all makes sense to me.

I notice at after the begin block you are just stating 'status' implying 'return status'   is explicitly saying 'return status' an un-ruby way of doing things? or is it just preference?

From: Tommy Bishop [mailto:notifications@github.com]
Sent: Friday, February 15, 2013 2:13 PM
To: intuit/simple_deploy
Cc: Young, Kevin
Subject: Re: [simple_deploy] To address issue 149,150 and 155 for simple deploy, Capistrano error handling (#154)


In lib/simple_deploy/stack/ssh.rb:

> @@ -20,7 +20,19 @@ def initialize(args)

>        def execute(args)

>          return false if @instances.nil? || @instances.empty?

>          create_execute_task args

> -        @task.execute

> +        begin

> +          @task.execute

> +        rescue ::Capistrano::CommandError => error

> +          @logger.error "Error running execute statement: #{error}"

> +          return false

> +        rescue ::Capistrano::ConnectionError => error

> +          @logger.error "Error connecting to instances: #{error}"

> +          return false

> +        rescue ::Capistrano::Error => error

> +          @logger.error "Error: #{error}"

> +          return false

> +        end

> +        @logger.info "Command executed against instance(s) successfully."

>        end

A couple of things.

There's a bit of duplication in here with all of the explicit returns. Instead, you can just set a var to a default value before you execute the task and override it depending on the situation. Then just return the var at the end of the method. Like:

def execute(args)

  ...

  status = false

  ....

  begin

    @task.execute

    status = true

  ....

  rescue Foo

    @logger.error 'boom'

  end



  status

end

Use empty lines to break up logical parts of the code. For example, I would probably have a new line after the initial guard ("return false if...."), before the begin, and after closing the last rescue statement. This makes it easier to scan and also helps isolate related chunks of code.

Typically you want to keep "happy path" code together. In this case, the successful log statement is 'far' from rest of the happy path code. If you scan down, you have to make mental context switches from happy path (no exceptions) to lots of failure paths (each rescue being a different one) back to happy path (the log call saying it was successful). Instead, you could just do:

begin

  ...

  @task.execute

  @logger.info "awesome"

rescue

  ...

end

Does this make sense?

—
Reply to this email directly or view it on GitHub<https://github.com/intuit/simple_deploy/pull/154/files#r3036234>.