-
Notifications
You must be signed in to change notification settings - Fork 72
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
Refactoring CLI #186
Refactoring CLI #186
Conversation
add_index :bars, :flew | ||
add_column :bars, :brew, :integer | ||
end | ||
end |
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.
Leftover migration?
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.
Whoops!
I'm kind of biased about the README generator because I put it in, but we found it pretty useful as a template for informative READMEs. I'd rather not blow away the specs, but I'm all for making them better. Though, this PR could get crowded with comments. If you could move the README generator removal and spec deletion to separate PR's, we can focus on those conversations better. |
I guess I'm not understanding how the README generator is used. Because a blank-ish README is already generated when you build a new Napa skeleton. So... are you using the Napa gem to create a README on a non-Napa project? Would you rather the README from the generator be moved to the README for the new/skeleton? |
I added the generator specs back in and updated them. |
We wanted people to have the option to use the README generator for their Napa project if they wanted to (we do), but we didn't want to enforce it. The generator creates a much more structured README that someone can fill out. It also creates a pending spec, which encourages you to fill it out. Though, again, this conversation should continue in a separate PR/issue. I'd take out the generator deletion from this PR. |
OK, the README generator is back! |
@brandonweiss thanks! |
@brandonweiss sorry for the lateness, I'll take a closer look at this soon. |
This is an awesome refactoring. I'm a fan. +1 from me. Just so others see the difference in how the CLI interacts: BEFORE: $ napa
Commands:
napa console [environment] # Start the Napa console
napa deploy [target] # Deploys A Service to a given target (i.e. production, staging, etc.)
napa generate api <api_name> # Create a Grape API, Model and Representer
napa generate migration <migration_name> [field[:type][:index] field[:type][:index]] # Create a Database Migration
napa generate readme # Create a formatted README
napa help [COMMAND] # Describe available commands or one specific command
napa new <app_name> [app_path] # Create a scaffold for a new Napa service
napa server # Start the Napa server
napa version # Shows the Napa version number
$ napa generate
Commands:
napa generate api <api_name> # Create a Grape API, Model and Entity
napa generate help [COMMAND] # Describe subcommands or one specific subcommand
napa generate migration <migration_name> [field[:type][:index] field[:type][:index]] # Create a Database Migration
napa generate readme AFTER: $ napa
Commands:
napa console [ENVIRONMENT] # Start the Napa console
napa deploy [TARGET] # Deploys A Service to a given target (i.e. production, staging, etc.)
napa generate [COMMAND] # Generate new code
napa help [COMMAND] # Describe available commands or one specific command
napa new <NAME> [PATH] # Create a new Napa application
napa server # Start the Napa server
napa version # Shows the Napa version number
$ napa generate
Commands:
napa generate api <NAME> # Create a Grape API, Model and Entity
napa generate help [COMMAND] # Describe subcommands or one specific subcommand
napa generate migration <NAME> [field[:type][:index] field[:type][:index]] # Create a Database Migration
napa generate readme |
This looks pretty solid, does it contain any breaking changes to the CLI interface? |
@darbyfrey |
oh, just kidding, that's just a naming change |
@brandonweiss merge conflicts 😭 |
That’s technically what it is.
I think this makes it easier to reason about as subcommands.
The usage of Thor seemed a little off to me. A few commands were Thor groups even though they didn’t need to be. Some commands were exposing helper methods as commands. I also refactored the file locations and namespaces to reflect the CLI itself a little better (grouping base and generate commands). It should be a bit easier to reason about now. I renamed the scaffold command to new (to reflect the actual command name).
👍 |
@brandonweiss you're awesome. +1 from me |
+1 looks good to me! thanks for cleaning that up @brandonweiss ! |
This is a large-ish refactoring of the CLI. I was having some issues chaining Thor commands while working on the model generator; I wanted it to invoke the migration generator but it wasn't working. I suspect it was the slightly unorthodox Thor organization, but I'm not positive.
If there's anything you're not digging let me know and I'll change or remove it.