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
[ENHANCEMENT] Support custom blueprint options in new and init commands #5329
[ENHANCEMENT] Support custom blueprint options in new and init commands #5329
Conversation
looks great. @trek would love your 👀 since the remote/external blueprint was your creation, want to make sure this is aligned with your goals for it. |
command.beforeRun(['app']); | ||
expect(pluck(command.availableOptions, 'name')).to.contain('custom-blueprint-option'); | ||
} finally { | ||
safeRestore(Blueprint, 'lookup'); |
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.
our other tests have the safeRestore
in the afterEach
so it's only needed once for all tests.
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.
Okay. I didn't want to stub out lookup
in every test, but you're suggesting that I put the safeRestore
in afterEach
, but keep the stub
in the test body?
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.
👍
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.
Yep, we added the safe version with a string arg so it only restores if it was stubbed.
1a150c5
to
648429b
Compare
kicking windows |
@bendemboski can you give some usage examples in the description? |
try{ | ||
blueprint = this.lookupBlueprint(rawArgs[0]); | ||
this.registerOptions(blueprint); | ||
} |
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.
style nitpic: } catch(e) {
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.
These are all my stylistic preferences as well, but I basically copied this method from here. Should I prefer style over consistency here?
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 would prefer the above mentioned style guide.
I would love to enforce style with JSCS or similar sometime soon, that way we can keep this consistent.
The new and init commands can be given a custom blueprint to use. This modifies those commands to be aware of blueprint-specific options and pass them through, like the generate command. This is handy in the near-term for custom app/addon blueprints that accept options, and could pave the way to add options to the build-in blueprints if that ever seems valuable. A few examples I can think of immediately are the server port and the root URL.
Safety first!
3fd979a
to
474bd65
Compare
single failure is unrelated and fixed upstream. Thanks for the contribution :) |
[ENHANCEMENT] Support custom blueprint options in new and init commands
The new and init commands can be given a custom blueprint to use. This modifies those commands to be aware of blueprint-specific options and pass them through, like the generate command.
This is handy in the near-term for custom app/addon blueprints that accept options, and could pave the way to add options to the built-in blueprints if that ever seems valuable. A few examples I can think of immediately are the server port and the root URL.
I didn't make any changes to the help output because the default blueprints don't support any custom options and
ember help new appName --blueprint=./my-blueprint
seems like a weird use case.