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

Dev #49

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Dev #49

wants to merge 3 commits into from

Conversation

phil043
Copy link

@phil043 phil043 commented Jul 7, 2015

Hi,

After a false start I've uploaded a version that enables --option=value style arguments. This is a fairly widely used method of passing in automatically generated arguments to a script (in our case our system uses it extensively).

It also adds a couple of tests for it.

There's a bug in the current dev branch Option::hasNeeds method that I've fixed - the in_array call wasn't using strict and was crashing on my tests as it was passing everything. This is happening because for whatever reason there's always an integer 0 in the "$definedOptions" array that is used in the method to check which options have been passed in allowing it to always return true.

The offending code is the first line in the below block:

if (!in_array($need, $definedOptions)) { <---- contains a 0 so matches any string in $need
                // The needed option has not been defined as a valid flag.
                $notFound[] = $need;
            } elseif (!$optionsList[$need]->getValue()) {
                // The needed option has been defined as a valid flag, but was
                // not pased in by the user.
                $notFound[] = $need;
            }

The first line should be (and in my version has been changed to be):

if (!in_array($need, $definedOptions, true)) {

There should probably be a proper method for it which loops through the array and does a proper comparison..

Finally I made a few small readability improvements in the Command::parse() method as it was getting confusing.

Adds test to cover --option=value arugments
Adds support for --option=value style arguments
Fixed needs failing when an argument with the value of "0" is passed
@Potherca
Copy link

This PR has some minor syntax issues in regards to NULL and FALSE not being lowercase and some method braces being moved onto the same line as the method declaration (basically not being PSR-2 compliant) but other than that, can we please haz this @nategood? 😺

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.

I am not a fan of the --flag=value syntax because it does not feel like a native command, so I don't feel any urgent need to include this as a feature.

case 2:
if (array_key_exists("equals", $matches) && !empty($matches["equals"])) {
$type = self::OPTION_TYPE_VERBOSE_EQUALS;
$name = str_replace("=", "", $name);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you move the regex group for the = outside of the name group, you would not need this str_replace, and you most likely do not need the extra identifier OPTION_TYPE_VERBOSE_EQUALS so it would be treated like any other long or short option.

$argument_value = $this->extractEqualsOptionValue($token);
} else {
// If the argument is of a --option value type
// the next token in the tokens array MUST be an "argument" and not another flag/option
Copy link
Contributor

Choose a reason for hiding this comment

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

Args don't have to have a value. Boolean flags are either off or on, and don't need to be supplied a value generally. If you are using an = on all of your options to denote a value than of course that will change the assumption that the next token must be a value for this option rather than another flag.

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.

3 participants