-
Notifications
You must be signed in to change notification settings - Fork 23
Console package #76
base: master
Are you sure you want to change the base?
Console package #76
Conversation
$this->flags = new InputBag(); | ||
$this->options = new InputBag(); | ||
|
||
$this->output = Output::getInstance(); |
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.
The input and output should probably be passed to the command from the console when the command is registered in the console.
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.
Fixed. Input and Output is now passed into the Console object which is passed into the command object when the command is added.
Added comments for basic syntax stuff. Still need to do another pass on the classes and architecture itself. Could also use more tests granular unit tests :P |
* | ||
* @return void | ||
*/ | ||
public function configure(): void; |
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.
Perhaps void
returns should actually be this
?
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 can, but this function should really only be called internally anyway. Thoughts?
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.
That's fine. I'm just not 100% sure how everything is used yet :P
… before registering them with the input. This allows the commands parameters to come before all registered global params for output, helpscreen, etc.
324356d
to
cc2fa54
Compare
I've updated the AsciiTable class to use the suggested output styling (similar to npm list). Run demo to check it out. |
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function handle(Input $input, Output $output, Next<Input, Output> $next): Output { |
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.
Does this function need to be added as an abstract method in the AbstractKernel
? (I just did inheritdoc for now just in case).
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 defined on the interface, so shouldn't be necessary.
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.
D'oh, didn't see it there.
Added a few more comments, we're almost there! The major issue I have is that there aren't too many tests. Whenever I write tests, I try and be as granular as possible and test every method on every class. If you could somehow mock all the classes and test as much as possible, that would be great. I'll also clone the repo and test it out on Windows again later today. |
@milesj, ping me when you're on IRC next. Can't seem to get the tests to run. |
Conflicts: composer.lock
Console package for building command line applications.