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

Added support for run() and created function for terminal errors #69

Closed
wants to merge 1 commit into from
Closed

Conversation

cwbeck
Copy link

@cwbeck cwbeck commented Oct 28, 2016

Added support for run() and created function for terminal errors

Added support for run() and created function for terminal errors
@nategood
Copy link
Owner

nategood commented Nov 9, 2016

Sorry if I'm being naive here, but what's the purpose of run? @cwbeck

@cwbeck
Copy link
Author

cwbeck commented Nov 10, 2016

__destruct() causes errors not to be thrown properly. This allows it to be called directly so errors can be caught. It's a PHP issue.

@NeoVance
Copy link
Contributor

-1 use parse.

Your try block should not be around the definition of the options. Errors will be thrown when the tokens are parsed. This happens when you access an option key for the first time.

$cmd->option('c');

try {
    $cmd['c']; // calls $cmd->parse()
} catch(Exception $e) {
    ...
}

Copy link
Contributor

@NeoVance NeoVance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The \ were just removed for phpdoc compatibility. Don't see any reason to bring them back.

The run is unnecessary. It is a clone of the parseIfNotParsed method. parse and isParsed are both public, so this can be handled in user code if desired.

I like the createTerminalError method addition.

@tolidano
Copy link

tolidano commented Jun 4, 2019

Ported to tolidano/commandox#21

@cwbeck cwbeck closed this Jul 4, 2019
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.

4 participants