Refactor repeatable+unique args and flag args

[DannyBen]

Closes #503

This PR changes how repeatable unique args and flag args are handled. The following changes were made:

Changes in Command model

1. Command#deep_commands now accepts the optional keyword arg include_self. If provided, self will be prepended to the resulting array. This is needed for the new Command#has_unique_args_or_flags? method.
2. New Command#has_unique_args_or_flags? which returns true if the command, or any child command has even one flag or flag arg defined as unique. This is needed for the conditional insertion of an array declaration in the run() function.

Changes in the generated bash script

1. The generated script's run() function will now define an additional associative array, unique_lookup, which is used as a tool for validating uniqueness. This is internal only, and users should not concern themselves with this array.
2. Input arguments in a repeatable+unique scenario, will be inserted as keys to the global unique_lkookup array. The key will contain the arg/flag name and its value - for example: --file:path/to/file.
3. Escaping is now done with escaped="$(printf '%q' "$1")".
4. The resulting array-string of all repeatable args or flag args is changed slightly. Instead of "a" "b c" it will be a b\ c. This is still compatible with the eval way of converting it back to array, but if anyone has used this string as is, it might break.

[DannyBen]

@pcrockett - please see the updated description of this PR, and review if you can.
The change was more involved than I had hoped, so I need a second eye on this before merging.

[pcrockett]

did a quick glance, looks good so far. i'll be interested in testing it a bit.

i've never personally used subcommands -- i suppose it's not a problem if a "parent" command defines a flag with the same name as the "child" command... lumping them all into the same arg makes sense to me in any case.

[DannyBen]

i suppose it's not a problem if a "parent" command defines a flag with the same name

It might be, if both are repeatable and unique and with the same name...
The parsing of the command line is done once, not per command - I did not see any other way.

The implementation is at the top edge of complexity I am willing to introduce for this fix, so if there are ways to simply it, I would love that, but if fixing this edge-of-the-edge case complicates it, I would avoid it.

---

Thinking about it more, I don't see this as an issue.
Having a flag with the same name for both parent and subcommand has no meaning already, since it will be provided as args['name_of_flag'] anyway.

Test bashly.yml

name: cli flags: - long: --file arg: path repeatable: true unique: true commands: - name: download flags: - long: --file arg: path repeatable: true unique: true

valid command line:

$ ./cli --file a --file a download --file a --file a 

output:

args: - ${args[--file]} = a 

[pcrockett]

i checked out the code and tested it on my machine, looked at the resulting bash diff.

looks great. only two comments are entirely nitpicky, feel free to ignore.

[pcrockett]

not an important detail, just a nitpicky inconsistency: single quotes on the left, no single quotes on the right.

Suggested change

	> args['{{ arg.name }}']="${args[{{ arg.name }}]} $escaped"
	> args['{{ arg.name }}']="${args['{{ arg.name }}']} $escaped"

same issue on line 22 below this

[pcrockett]

i notice this section of code produces something like:

escaped="$(printf '%q' "$1")" if [[ -z ${args['term']+x} ]]; then args['term']="$escaped" unique_lookup["term:$escaped"]=1 shift elif [[ -z "${unique_lookup["term:$escaped"]:-}" ]]; then args['term']="${args[term]} $escaped" unique_lookup["term:$escaped"]=1 shift else shift fi

dunno if it would be more complex, or less, to simplify the logic a bit:

escaped="$(printf '%q' "$1")" if [[ -z ${args['term']+x} ]]; then args['term']="$escaped" unique_lookup["term:$escaped"]=1 elif [[ -z "${unique_lookup["term:$escaped"]:-}" ]]; then args['term']="${args[term]} $escaped" unique_lookup["term:$escaped"]=1 fi shift

[DannyBen]

Thanks.

I am updating both your comments.
In regards to the last comment, there was also another issue when there were more than one args defined. The assignment of escaped="$(printf '%q' "$1")" appeared more than once, and not in the correct place.

This YAML demonstrated it.

name: cli args: - name: path - name: term repeatable: true unique: true

Bugged

 *) # :command.parse_requirements_case # :command.parse_requirements_case_repeatable escaped="$(printf '%q' "$1")" if [[ -z ${args['path']+x} ]]; then args['path']=$1 shift escaped="$(printf '%q' "$1")" elif [[ -z ${args['term']+x} ]]; then args['term']="$escaped" unique_lookup["term:$escaped"]=1 shift elif [[ -z "${unique_lookup["term:$escaped"]:-}" ]]; then args['term']="${args[term]} $escaped" unique_lookup["term:$escaped"]=1 shift else shift fi ;;

Fixed

 *) # :command.parse_requirements_case # :command.parse_requirements_case_repeatable escaped="$(printf '%q' "$1")" if [[ -z ${args['path']+x} ]]; then args['path']="$1" elif [[ -z ${args['term']+x} ]]; then args['term']="$escaped" unique_lookup["term:$escaped"]=1 elif [[ -z "${unique_lookup["term:$escaped"]:-}" ]]; then args['term']="${args['term']} $escaped" unique_lookup["term:$escaped"]=1 fi shift ;;

[pcrockett]

Oo, good catch. Surprised a test didn't catch it. Worth adding to the test suite?

[DannyBen]

Oo, good catch. Surprised a test didn't catch it. Worth adding to the test suite?

The code it generates is valid, so no way to catch it failing (but maybe I can catch it failing the shfmt test...).
The truth is that the entire parse_requirements is probably the most complex section of the code generation.

Are we ready to merge?

[pcrockett]

Oh, right. Just escaping wouldn't affect global state... Yes. Merge that sucker.