Up and attach #27
Up and attach #27
Conversation
…ces. Also got rid of the alsiases because it's confusion there's bot an instance variable and a class instance variable called opts
|
@davidsiaw please review this |
|
This actually fixes #16 |
lib/kaiser/cli.rb
Outdated
| end | ||
| end | ||
|
|
||
| attr_reader :opts |
There was a problem hiding this comment.
This bunch of things is a great thing to abstract into a module. If you define this in a module, say a CliOptions module, you can extend CliOptions in the class Cli and it will be easier to see whats going on.
At the moment its really easy to confuse :opts and options.
Also with a CliOptions module renaming @options to @cli_options would make making the link easier.
There was a problem hiding this comment.
Potentially you can also add it to only commands that require options if you do it this way. So it looks a lot less like magic.
There was a problem hiding this comment.
Yeah I was also worried about opts and options being confusing. Especially since one is a class instance variable and the other is an instance variable. I'll put them in the module. I also like the idea of less magic. :)
There was a problem hiding this comment.
I do think it's better to keep it in the superclass because this line over here relies on the cmd.class.options method existing and returning an array.
opts = cmd.define_options(global_opts + cmd.class.options)I feel like this is more elegant than writing a check to see if the module was extended or not.
lib/kaiser/cli.rb
Outdated
| @@ -29,7 +44,7 @@ def define_options(global_opts = []) | |||
| # the scope to Optimist::Parser. We can still reference variables but we can't | |||
| # call instance methods of a Kaiser::Cli class. | |||
| u = usage | |||
| Optimist.options do | |||
| @opts = Optimist.options do | |||
There was a problem hiding this comment.
Instead of making this global, can we just pass it in as a parameter to execute?
There was a problem hiding this comment.
I thought about too at first but the thing is execute calls a lot of private methods that are often shared between commands. We would have to pass the options to every single one.
Or I guess you could only pass them to the ones that actually make use of them which could make it more transparent. You could see at a glance which method uses options and which one does not.
I started writing this comment as a counter-argument but finished it thinking maybe you're right. lol
It would be less magic that way too.
…instead of setting an instance variable
When running
kaiser upfollowed bykaiser attach, it starts a docker container with the app in it, only to shut it down immediately and start up another one with an "attached" app in it.This process has been bothering me a little for a while already so I quickly added a
-aor--attachoption to thekaiser upcommand.Now the following command sets up the database and immediately attaches the source code in the current directory.