-
Notifications
You must be signed in to change notification settings - Fork 11.9k
feat(@angular/cli): Update command runner. #9707
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
Conversation
327d9a1
to
e9c854d
Compare
94aeda4
to
2062349
Compare
ef7c146
to
bdf816a
Compare
7ab5470
to
c15a808
Compare
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.
Some first comments.
`Error 418: As Awesome As Can Get.` | ||
`Error 418: As Awesome As Can Get.`, | ||
`I spy with my little eye a great developer!`, | ||
`noop... already awesome.` |
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.
Capital letter?
project: environment.project, | ||
ui: this.ui, | ||
}; | ||
return runCommand(environment.commands, environment.cliArgs, logger, context) |
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.
Are we doing await
/async
in this 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.
Yes, I can refactor, but I wanted it working first.
@@ -24,6 +24,8 @@ module.exports = function(commands, commandName, commandArgs, optionHash) { | |||
// Attempt to find command in ember-cli core commands | |||
let command = findCommand(commands, commandName); | |||
|
|||
|
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.
Remove empty new lines.
} | ||
|
||
validate(_options: any): boolean | Promise<boolean> { | ||
return true; |
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.
No need for this, right?
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's a base implementation that can be overridden, but the runner relies on a boolean being returned. Not all commands will implement validate.
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.
Sorry I pointed to the wrong file. the Doc command still has an empty override.
|
||
availableOptions: [ | ||
export default class GetCommand extends Command { | ||
public name = 'get'; |
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.
readonly?
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.
Won't hurt anything by adding it.
When this PR gets merged, this devkit PR should be merged too: angular/devkit#481 |
93d2afd
to
b9df667
Compare
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. We can rework stuff with the TODOs in there. Cheers!
This change removes the dependency of the ember-cli command structure.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This change removes the dependency of the ember-cli command structure.