-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Enable ember generate <blueprint> --help for individual blueprint docs #3316
Conversation
Some previous discussion and ideas regarding the blueprint-parts of this: #1279 (comment) |
7008488
to
3eb5a1a
Compare
I've removed the help option from I took an initial crack at the model help text, but it needs some work. Input on this part in particular would be great. I'm thinking that blueprint help should probably have some convenience methods to handle formatting, and that should probably live on the blueprint model somewhere. The current Requested ember-cli commands:
ember generate <blueprint> <options...>
Generates new code from blueprints.
aliases: g
--dry-run (Boolean) (Default: false)
aliases: -d
--verbose (Boolean) (Default: false)
aliases: -v
--pod (Boolean) (Default: false)
aliases: -p
model <name> <attr:type>
Generates an ember-data model.
You may generate models with as many attrs as you would like to pass. The following attribute types are supported:
<attr-name>
<attr-name>:array
<attr-name>:boolean
<attr-name>:date
<attr-name>:object
<attr-name>:number
<attr-name>:string
<attr-name>:belongs-to:<model-name>
<attr-name>:has-many:<model-name>
For instance: `ember generate model taco filling:belongs-to:protein toppings:has-many:toppings name:string price:number misc`
would result in the following model:
import DS from 'ember-data';
export default DS.Model.extend({
filling: DS.belongsTo('protein'),
toppings: DS.hasMany('topping')
name: DS.attr('string'),
price: DS.attr('number')
misc: DS.attr()
}); |
@trabus This looks nice. Right now, ember points you to use the |
Actually |
var commandName = args.shift(); | ||
var commandArgs = args; | ||
return Promise.hash(environment) | ||
.then(function(environment) { |
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.
It becomes harder to review the changes because you're changing the syntnax here (moving then
to a new line and indenting the block). I'd put that kind of stuff in a separate pr.
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've reverted the formatting for now.
a5bf778
to
68dd6f5
Compare
I don't know why appveyor is failing, but it's bailing before even running the tests. |
I really like the functionality this provides. I'm not sure whether the current JS implementation or a markdown file as suggested in #1279 should be preferred though. I also think we might be able to simplify/clarify the implementation a little bit (to make it more transparent what's happening). |
If we can throw a markdown file in a blueprint, then parse that into color text, I'm absolutely for that (it would be much easier and better than string concatenation like we're currently doing). I'm sure something like that is possible, but I'm not familiar with a way to do it myself. I like how projects like sails have README.md files in folders for some of the files, and they also use them for their docs I've noticed. |
Seems like the markdown part could come later, if desirable. This adds lots of value as it currently stands. |
Now we just need to go through and add |
I could see the |
Totally.
Do you want to add all the detailed help in this PR, or have it happen piecemeal in PRs that follow? I’d say LGTM and the open it up to the community to drive detailed documentation of blueprints. |
I vote piecemeal. Maybe I can get the markdown parsing in sooner than later, and we'd save ourselves from having to write the help docs twice. |
Cool. LGTM. Looks like the AppVeyor failure is a problem installing Phantom. Can someone with the keys re-run the build? |
@johanneswuerbach awesome thank you. |
@@ -61,6 +61,9 @@ function Command() { | |||
// Options properties | |||
this.availableOptions = this.availableOptions || []; | |||
this.anonymousOptions = this.anonymousOptions || []; | |||
// push in help option for all commands | |||
// this.availableOptions.push({ name: 'help', type: Boolean, default: false, aliases: ['h'] }); | |||
|
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.
Can we delete these lines altogether?
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.
What lines?
Sorry, forgot about those after I commented them out. ;)
8605df0
to
de0e3a8
Compare
Rebased and re-ran travis (after a timeout on 0.12). AppVeyor seems okay aside from the timeout, but that's also happening for master. |
This PR fixes #2553, adding the ability to add
printDetailedHelp
to blueprints.I've extracted the help logic from
lookup-command.js
and added a then in the command processing promise chain incli.js
which allows a command to exit before running if the-h
or--help
option is present. From there the help command is run with the original options passed in (in addition to the command name).I've added the help option to
availableOptions
for all commands, which as a side effect lists the--help
option as an availableOption for every command. For example:I can remove this if desired, which would go back to the current setup where the
--help
option is assumed present.I still need to write tests for this, and I would like to follow this PR up with adding
printDetailedHelp
to all of the built-in blueprints that could use it. I've mainly put this up for review as I progress on it.