New features and some fixes - Variadic arguments flexibility and grouping, Negative numbers #94

shlomohass · 2023-09-29T07:47:27Z

This is the result of / and fixes #93

What has changed:

Introduced a new Tokenizer class to enable a more advanced syntax to be parsed (Normalizer now only handles the value filtering)

Negative numbers are now supported and will be auto-detected and not treated as options.
As discussed, I implemented a "variadic boundaries" syntax, which offers more flexibility and the use of multiple variadic arguments/options.
a bug with options that are defined only with a long name and are variadic ->option("--names [items...]") is now working as expected.
minor improvements and fixes to satisfy linters and phpstan.

# Now, variadic arguments can be explicitly used : 
$ app cmd arg11 arg12 --opt [ val1 val2 ] # 👍 
$ app cmd [arg11 arg12] --opt [val1 val2] # 👍 
$ app cmd arg11 arg12 --opt val1 val2 # 👍 
$ app cmd [arg11 arg12] [arg21 arg22] --opt val1 val2 # 👍 
$ app cmd arg1 arg2 --flags [ -- -a -b -c ] --opt val1 val2 # amazingly this one is working also 👍 

#Negative numbers:
$ app scan --offset -5 # 👍

Tested on/with:

I made a new test file, AdvacedArgsTest.php with many edge cases and crazy scenarios.
win 10 - PHP 8.1 - bash, cmd
ubuntu 22 - PHP 8.2
mint - PHP 8.2
PHPunit and real applications with user input.

Notes:

I did my best to keep backward compatibility so it should work with older implementations. That said, you should consider (in case you approve this pull) bumping the major version as it introduces new features and, in some rare and weird cases, it may break the current implementations - maybe someone relies on the "last variadic" error 😲
I tested it in real-world applications (mine), and it worked out of the box - It will be great if you can test it against your own apps just to be sure.

I have some more ideas for future improvements 😄

…t I keep it for future use.

…gs that pepole are complaining about

…php7 is dropped

…ax features

…panded to constants

adhocore · 2023-09-29T11:10:57Z

oh, just saw this and looks great. i will check indepth from PC soon. thanks for working out the PR

shlomohass · 2023-09-29T13:05:16Z

@adhocore Take your time and test it carefully. I really did my best to cover any weird (sensible) combination, but you can never be sure.

src/Helper/Polyfill.php

src/Input/Option.php

src/Input/Parser.php

adhocore · 2023-10-01T09:34:21Z

src/Input/Parser.php

+
+                $this->set(null, $arg->value());
+
+            } else {


the if/else looks a bit oddly. can we refactor it? maybe a method extraction for non-constant case

What do you mean? I'm still determining...

I had another version which was recursive and messy.
I think the naive way of simple, expressive conditions is the simplest and easiest way (and performant)

Can you elaborate on that?

src/Input/Parser.php

adhocore · 2023-10-01T09:37:23Z

src/Input/Parser.php

-        if (!$argument = reset($this->_arguments)) {
-            return $this->set(null, $arg);
+        // Its variadic, so we need to collect all the remaining args:
+        if ($argument->variadic() && $arg->isConstant()) {


this block should be a different method

adhocore · 2023-10-01T09:37:40Z

src/Input/Parser.php

        }

-        $this->setValue($argument, $arg);
+        // Its variadic, and we have a variadic grouped arg:
+        if ($argument->variadic() && $arg->isVariadic()) {


should be extracted to different method

src/Input/Parser.php

adhocore · 2023-10-01T09:41:02Z

src/Input/Parser.php

+        if ($option->variadic() && $next->isVariadic()) {
+
+            foreach ($next->nested as $token) {
+                if ($token->isConstant()) {


i see many code repetition of this nature:

if (token isConstant) { setValue } else { throw ... }

--- should be extracted to some helper (setConstantOrFail()) that can be used like this:
$this->setConstantOrFail($token)

This is because I want to avoid recursion (as much as possible), and as a general rule I normally don't add more abstraction, especially for internal code which probably will not be used anywhere else...
That said I can add more methods if it's a design rule.

this loop is convoluted so if the command definitions for options don't contain any variadic option at all, we should have a way to just use older approach of parsing for performance reasons. i think variadic options are only a small percentage of non variadic options.

I'm sorry I don't really understand the point.

My reasoning is:

The number of tokens we will iterate through will most of the time, be meaningless, I mean it's a CLI arguments parser...

I wanted to reduce function calls as much as possible (for performance) and I wanted to avoid unnecessary Iterations/function calls.

I chose this pattern because it's simple, bulletproof, and only consumes/loops/passes values if it's absolutely needed.

In this case unless $option->variadic() which means it was declared as variadic and I have a specific $next->isVariadic() (which means it's a specific variadic group i.e. [ 1 2 3 ]) - no loop will be reached.

If this is the case all the "grouped" arguments will be consumed in one pass.

Please elaborate

src/Input/Tokenizer.php

adhocore · 2023-10-01T09:51:26Z

src/Input/Tokenizer.php

+        }
+        // If its a single hyphen, maybe its a negative number:
+        if (
+            ($arg[0] ?? '') === '-'


btw we cant assume -N where N is number, to be a number it can be a option as well

eg curl uses -1 -2 -3 as option for ssl v1 to v3

This is pretty rare and usually confusing, I can think of a way to support numerical names for options by checking first if the current identified number is defined as an option.
That said, it still introduces some extra complexity and weirdness : -10 -12 ...

adhocore · 2023-10-01T09:54:47Z

src/Input/Tokenizer.php

+                    case Token::TYPE_LONG:
+                    case Token::TYPE_CONSTANT:
+                        if ($variadic) {
+                            $this->tokens[count($this->tokens) - 1]->addNested($token);


the nesting goes quite deep: class -> func -> foreach -> foreach -> switch -> case -> if
probably need to optimize, specially loop inside loop

What is wrong with that? It's a lot cheaper than function/method calls...
1 scope block + 4 code blocks are not considered as deep I think.

src/Input/Token.php

adhocore

hello @shlomohass so too many things and big scope, i think i have left enough comments in review for now. (will check again later again with more comments lol).
quick update we can do here besides addressing reviews; i guess, is firstly using a proper linting/php-cs-fixer and secondly removing code-comments that is not absolutely necessary. thank you

shlomohass · 2023-10-01T13:24:30Z

@adhocore Sure I'll start making the edits. I thought the CodeFactor has autofix for all the empty lines and so on... I will do it on my branch and update the pull.

shlomohass · 2023-10-02T08:32:38Z

Those Are the inputs:

On:

Ubuntu 22
PHP 8.2.1

I'm sure it can be improved even more...

shlomohass added 14 commits September 29, 2023 06:00

New Tokenize and Token classes for argv parsing Striped Normalizer bu…

d78e60c

…t I keep it for future use.

added phpstan to dev just to check and later fix all the crazy warnin…

fc20eb0

…gs that pepole are complaining about

small phpstan fixes that actually make sense

53d1980

Helper class to support older php versions... should be removed when …

1d69be1

…php7 is dropped

Parser new version to use the tokenizer and support the new argv synt…

f7a305b

…ax features

adapted to be used with the new tokenizer and parser

7c0a276

small phpstan fix with actionCalled not defined

cdc128c

New test + Removed invalid test for the extended argv syntax parser

59a1b9b

removed comments

a459c0d

Merge branch 'main' of github.com:shlomohass/php-cli

2149fa4

small fix filter should return int

7a1ea62

simplified option parsing and removed literal handling as they are ex…

8af5946

…panded to constants

added "insane" tests just to make sure

8778eaf

added line break when an exception is raised

811c71d

shlomohass mentioned this pull request Sep 29, 2023

Negative argument values and optional variadic arguments / options. #93

Open

shlomohass changed the title ~~New features and some fixes - Variadic arguments flexibility and grouping, Negative numbers~~ #93 New features and some fixes - Variadic arguments flexibility and grouping, Negative numbers Sep 29, 2023

shlomohass changed the title ~~#93 New features and some fixes - Variadic arguments flexibility and grouping, Negative numbers~~ New features and some fixes - Variadic arguments flexibility and grouping, Negative numbers Sep 29, 2023