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

private -> protected accessibility for overloading #31

Closed
wants to merge 1 commit into from
Closed

private -> protected accessibility for overloading #31

wants to merge 1 commit into from

Conversation

bpacholek
Copy link

In order to develop Single command applications it is required to use the defaultCommand. To do we need to overload the ExpressionParser. In the method parse we need to disable the line:

$name = array_shift($tokens);

and later passing of the name:

        return [
        //    'name' => $name,
            'arguments' => $arguments,
            'options' => $options,
        ];

There would be no problem in that, but since methods like parseOption, startsWith (etc.) are private it is required to repeat them in the child class. Having them protected would allow use of inheritance:

class SingleCommandExpressionParser extends ExpressionParser

Going further, we need to update the method createCommand, but we also need to overload command as it calls createCommand of the parent scope if not overloaded. If overloaded then we need to repeat assertCallableIsValid (etc.) again to be accessible.

@tqmz tqmz mentioned this pull request Sep 27, 2016
@mnapoli
Copy link
Owner

mnapoli commented Sep 29, 2016

I'm not a huge fan of this change, I'm having trouble understanding why it's necessary to change everything to protected. Can the SingleCommandApplication take the command in its constructor? If so, do we still need to overload everything? Make with an example it would be clearer.

@bpacholek
Copy link
Author

Another option would be to detect if command name was parsed in the constructor and modify both expression parser and commands to behave differently then, right.

Will provide a new PR.

@bpacholek bpacholek closed this Oct 24, 2016
This pull request was closed.
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.

2 participants