Fix compiler flags with optional arg eating following flags

[yb66]

Hi all,

I don't know if this should be part of #5338, #8656 or #11136, and I see a PR in #9909, so it's possible there's some overlap. What I do know is that I had an itch to scratch because my CLI app has defined within it several flags with optional arguments that would then eat following arguments if not provided a value.

All existing specs pass on my machine.

Let me know if there's anything untoward.

Regards,
iain

[yb66]

Btw, the check that is failing is a formatting check in the spec file. Running crystal tool format --check spec/std/option_parser_spec.cr gives me formatting './spec/std/option_parser_spec.cr' produced changes. I've no idea what that means, any guidance on using that tool would be welcome.

Regards,
iain

[asterite]

Hi @yb66

We have a code formatter in place, and all code in this repository must follow that format.

You can run:

crystal tool format spec/std/option_parser_spec.cr

that will change the file to follow that format. Then commit and push those changes. Thank you!

[yb66]

Thanks @asterite, I see it's already in the guide, I missed that, sorry. I'm installing the hook now!

Regards,
iain

[straight-shoota]

The PR should have a clear description of the proposed change. Only after looking into the code, I realized what it is about.

In short words, if --foo is a flag with optional value, it makes --foo --bar parse --foo without a value (and --bar as the next flag). Currently, --bar would be considered a value of --foo.

So this addresses a part of #5338. This might even have worked like this before some refactoring, but I'm not sure.

I think this is probably a useful improvement. But it is not a generic solution. For example, it can't tell an optional value from a positional argument.

[straight-shoota]

I don't follow the reasoning behind the regex. Doesn't the last clause (^(.+)) make it match any string (except the empty one)?

And what is NotThis|NotThat|GoAway|(WeWantThis) supposed to mean?

[yb66]

NotThis|NotThat|GoAway|(WeWantThis) is a description of the regex. Every clause prior to the last one is something not to match on, the last clause matches things that are wanted. Thus, the two can be differentiated by whether they were captured or not. That's why the $1 is the last part of the conditional.

A longer explanation is given here, it's called The Greatest Regex Trick Ever (a title I agree with) and is designed to be the simplest way to do this kind of job. If there's a better way, I'm all ears.

[straight-shoota]

Ah I think I understand it. But I'm not particularly fond of using a regex that's really hard to reason about.

But before looking into possible alternative expressions, I'd like to talk about the intention.
Could it just be !value.starts_with?("-")? Why is -- explicitly excluded?

[yb66]

The complexity of the problem is increased because of handling the --. That's a special case because of #8937, specifically a case like this:

But to do it correctly, the behavior must depend on context. If we're currently expecting an option value, the next token must be recognized as that regardless of its content.

E.g. Git recognizes this as an option correctly:
git commit -m -- (create a commit with the message "--")

It's quite possible there's a simpler regex but I'd say that's straightforward to spot once you know that trick. It's more likely that it can be simplified by not using a regex and using more code.

How about using the whitespace regex flag to add in some comments? From line 382…

Suggested change

	# e.g. Do not match `-g` or `--g-long` but do match `--`
	# NotThis|NotThat|GoAway|(WeWantThis)
	if value && value =~ /^\-(?!\-)|^\-\-\S|^(.+)/ && $1?
	value = args[arg_index + 1]?
	# We don't want short or long flags as values here
	# but sometimes double dashes alone are ok.
	# The easiest way to handle is to *capture*
	# what we want and drop all other matches.
	# e.g. not `-g` or `--g-long` but do capture `--`
	# The logic is:
	# `/NotThis|NotThat|AndNotThis...|(WeWantThis)/`
	flag_pattern = /
	^\-(?!\-) # This matches short flags e.g. -g
	|
	^\-\-\S # This matches long flags
	|
	^(.+) # This captures what we want
	/x
	# If there is a capture ($1 is true)
	# then this condition is true
	if value && value =~ flag_pattern && $1?

The specs pass for that.

[straight-shoota]

Thanks, the explanatory comments are really helpful.

But I'm pretty sure a non-regex implementation would be easier to understand and even more efficient.
I believe !value.starts_with?("-") || value == "-" || value == "--" should be pretty much equivalent.

[yb66]

Well, I'm not going to die on hill of "regex are easy to grok" 😆 though I do use them an awful lot, but your questions led me to a (hopefully) much better solution.

I realised that the regex would fail if something looked like an option e.g. --not-an-option--, and I added a spec for that and it did indeed fail. The answer…

              if value && [email protected]_key?(value)

Just check the handler's hash instead, all the flags are in there already! It passes all the specs.

[straight-shoota]

Yeah, I suppose that's probably the best solution.

[yb66]

The PR should have a clear description of the proposed change. Only after looking into the code, I realized what it is about.

My apologies.

But it is not a generic solution. For example, it can't tell an optional value from a positional argument.

I think this comes down to our needs here. You're looking to improve the language/tools as a whole long term and without needing to return to things. I want that too but I also have a need for this specific fix right now, so that's the focus of this patch. I haven't looked at OptionParser as a whole.

Regards,
iain

[beta-ziliani]

🚀 thanks @yb66 !

[HertzDevil]

Related: #11546 will make optional flags skip any subsequent argument, not just flags.