-
Notifications
You must be signed in to change notification settings - Fork 1
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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I did not know how to do this properly but you came up with a good solution.
Just some comments that could potentially improve the clarity of the new functionality.
lib/kaiser/cli.rb
Outdated
end | ||
end | ||
|
||
attr_reader :opts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of making this global, can we just pass it in as a parameter to execute
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
When running
kaiser up
followed 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
-a
or--attach
option to thekaiser up
command.Now the following command sets up the database and immediately attaches the source code in the current directory.