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.

Sign up for GitHub

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

Jump to bottom

Error when value is NULL #84

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Error when value is NULL #84

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/Commando/Command.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,9 +439,14 @@ public function parse()
// the next token MUST be an "argument" and not another flag/option
$token = array_shift($tokens);
list($val, $type) = $this->_parseOption($token);
if ($type !== self::OPTION_TYPE_ARGUMENT)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is here because we expect that if the user is supplying the long/short flag that expects an argument, that they would also supply the argument value. Optional flags can still have a default value, but if they user just wants the default value, they don't need to supply to flag on the command line at all. Take this excerpt from man grep for example.

2.1.2 Matching Control
-e pattern
--regexp=pattern
Use pattern as the pattern. If this option is used multiple times or is combined with the -f (--file) option, search for all patterns given. (-e is specified by POSIX.)

-f file
--file=file
Obtain patterns from file, one per line. If this option is used multiple times or is combined with the -e (--regexp) option, search for all patterns given. The empty file contains zero patterns, and therefore matches nothing. (-f is specified by POSIX.)

-i
-y
--ignore-case
Ignore case distinctions, so that characters that differ only in case match each other. Although this is straightforward when letters differ in case only via lowercase-uppercase pairs, the behavior is unspecified in other situations. For example, uppercase “S” has an unusual lowercase counterpart “ſ” (Unicode character U+017F, LATIN SMALL LETTER LONG S) in many locales, and it is unspecified whether this unusual character matches “S” or “s” even though uppercasing it yields “S”. Another example: the lowercase German letter “ß” (U+00DF, LATIN SMALL LETTER SHARP S) is normally capitalized as the two-character string “SS” but it does not match “SS”, and it might not match the uppercase letter “ẞ” (U+1E9E, LATIN CAPITAL LETTER SHARP S) even though lowercasing the latter yields the former.

-y is an obsolete synonym that is provided for compatibility. (-i is specified by POSIX.)

-v
--invert-match
Invert the sense of matching, to select non-matching lines. (-v is specified by POSIX.)

--regexp/-e and --file/-f expect an argument to be supplied. This is the default behavior for all flags in Commando.

--ignore-case/-i and --invert-match/-v do not expect a value to be supplied as an argument, but this is not because they have some optional argument with a value that can be defaulted per se. It is because they are a Boolean type of flag. They are either supplied, or they are not. Their mere presence on in the command signals all that the application needs to know in order to fulfill the desired behavior.

Having a flag that is a hybrid Boolean, and can be supplied on the command line with or without an argument is something else entirely. Like --color.

--color[=WHEN]
--colour[=WHEN]
Surround the matched (non-empty) strings, matching lines, context lines, file names, line numbers, byte offsets, and separators (for fields and groups of context lines) with escape sequences to display them in color on the terminal. The colors are defined by the environment variable GREP_COLORS and default to ‘ms=01;31:mc=01;31:sl=:cx=:fn=35:ln=32:bn=32:se=36’ for bold red matched text, magenta file names, green line numbers, green byte offsets, cyan separators, and default terminal colors otherwise. The deprecated environment variable GREP_COLOR is still supported, but its setting does not have priority; it defaults to ‘01;31’ (bold red) which only covers the color for matched text. WHEN is ‘never’, ‘always’, or ‘auto’.

For something like this, I would probably want to add a property on Option to allow this behavior rather than making it the default behavior for when Option::$default is set. I think it would be better if there was another way to set a flag's argument as optional. This would allow us to have a flag like color, which has a default value of never and when supplied without arguments changes the value to auto, but can take additional argument values like always. Color kind of operates like an Enum so maybe that can be the property name. Check if Option::isEnum() etc...

throw new \Exception(sprintf('Unable to parse option %s: Expected an argument', $token));
$keyvals[$name] = $val;

if ($type !== self::OPTION_TYPE_ARGUMENT && $option->getDefault() === null) {
throw new \Exception(sprintf('Unable to parse option %s: Expected an argument or default', $token));
}

$keyvals[$name] = ($type !== self::OPTION_TYPE_ARGUMENT)
? $option->getDefault()
: $val;
}
}
}
Expand Down