Skip to content
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

Merged
merged 6 commits into from
Mar 17, 2015
Merged

Refactoring CLI #186

merged 6 commits into from
Mar 17, 2015

Conversation

brandonweiss
Copy link
Contributor

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.

add_index :bars, :flew
add_column :bars, :brew, :integer
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops!

@shaqq
Copy link
Contributor

shaqq commented Feb 17, 2015

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.

@brandonweiss
Copy link
Contributor Author

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?

@brandonweiss
Copy link
Contributor Author

I added the generator specs back in and updated them.

@shaqq
Copy link
Contributor

shaqq commented Feb 18, 2015

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.

@brandonweiss
Copy link
Contributor Author

OK, the README generator is back!

@shaqq
Copy link
Contributor

shaqq commented Feb 19, 2015

@brandonweiss thanks!

@shaqq
Copy link
Contributor

shaqq commented Mar 6, 2015

@brandonweiss sorry for the lateness, I'll take a closer look at this soon.

@shaqq
Copy link
Contributor

shaqq commented Mar 9, 2015

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

@darbyfrey
Copy link
Contributor

This looks pretty solid, does it contain any breaking changes to the CLI interface?

@shaqq
Copy link
Contributor

shaqq commented Mar 9, 2015

@darbyfrey napa scaffold to napa new, i believe

@shaqq
Copy link
Contributor

shaqq commented Mar 9, 2015

oh, just kidding, that's just a naming change

@shaqq
Copy link
Contributor

shaqq commented Mar 16, 2015

@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
Copy link
Contributor Author

👍

@shaqq
Copy link
Contributor

shaqq commented Mar 17, 2015

@brandonweiss you're awesome.

+1 from me

@itsbagpack
Copy link
Contributor

+1 looks good to me! thanks for cleaning that up @brandonweiss !

shaqq added a commit that referenced this pull request Mar 17, 2015
@shaqq shaqq merged commit 2fb1500 into bellycard:master Mar 17, 2015
@brandonweiss brandonweiss deleted the refactoring-cli branch March 17, 2015 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants