Incorrect behavior of flags with optional value

[ya-evgeniy]

I registered a command with a flag that takes an optional value.

CommandElement flagValue = GenericArguments.bool(Text.of("flag_value"));
CommandElement optionalFlagValue = GenericArguments.optional(flagValue);
CommandFlags.Builder flags = GenericArguments.flags();
flags.valueFlag(optionalFlagValue, "-flag");
CommandElement commandElement = flags.buildWith(GenericArguments.none());

As well as a handler that displays data on the flag. ps: Just in case, I tried to use "-".

Text.of("has flag_value ", args.hasAny("flag_value") )
Text.of("has -flag_value ", args.hasAny("-flag_value"))
Text.of("has flag ", args.hasAny("flag"))
Text.of("has -flag ", args.hasAny("-flag"))
Text.of("get flag_value ", args.getOne("flag_value"))
Text.of("get -flag_value ", args.getOne("-flag_value"))
Text.of("get flag ", args.getOne("flag"))
Text.of("get -flag ", args.getOne("-flag"))

For command "/test_flags --flag", the result can be seen in the screenshot below.

Expected behavior:
By my logic, args.hasAny("flag") should have returned true, and args.getOne("flag_value") should return Optional.empty(). If I am mistaken in some way, then correct me.

Server: spongevanilla-1.12.2-7.1.5

p.s. The text is translated by a translator.

[WillBAnders]

The way flags work are a bit different than you expect. Flags are not themselves CommandElements, but each flag uses an element to handle it's parsing. A value flag is one where you provide this element so it can parse additional values after the flag. The "flag" parameter is only used to identify the flag and is not part of this element, thus it is not included in the final context.

Additionally, flags should not be combined with optional elements because it adds multiple layers of optional. Flags are inheritably optional, so you should stick to using --flag value for present values and `` (empty) when the flag is not defined. As such, of the calls given only hasKey("flag_value") and `getOne("flag_value")` will work.

The idea of including a way to determine what flags were specified themselves is definitely meritable and worth discussion for API 8.

[dualspiral]

I wondered how I'd missed this in the first place, but then realised I was on holiday at the time.

While @WillBAnders's response is mostly correct, there is one thing that I'd contest:

Additionally, flags should not be combined with optional elements because it adds multiple layers of optional.

This is true, except in the case where you might want to be able to omit a value so that specifying a flag has a "default" value. That's clearly what is wanted here (--flag should act as --flag true), but the element does not put anything into the context. In this case, you'd use the optional overload that allows you to specify a default.

In other words, changing:

CommandElement optionalFlagValue = GenericArguments.optional(flagValue);

to

CommandElement optionalFlagValue = GenericArguments.optional(flagValue, true);

would resolve this issue as required.

A note on how CommandFlags.flag(...) is implemented and how it returns a value in the context - it effectively redirects to CommandFlags(GenericArguments.markTrue(firstalias), ...), then works as Will explains above. It is implemented in a consistent way, but I can see where the confusion comes from.

As a stop gap, probably could add something to the CommandContext that provides the flags that were detected and acted upon, but I suspect it's not entirely necessary.

[WillBAnders]

That's a good clarification. Optional elements themselves are fine with things like defaults. The issues I was referring to only happen when you have layers of non-default optionals - like combining multiple nulls, it's easy to loose context for what that means.

I'm all in for adding a markTrue element on top of value flags. This could impact existing plugins if they use the name of the flag for another element, but does not impact the case of --flag <flag> (where the flag value is not optional or has a default) as it would replace the earlier one. I'd be happy to do this if it's a worthwhile change as I believe the additonal context here is really helpful.

[dualspiral]

That's not what I was suggesting. I was thinking you could have something outside of the whole getOne(...) context, a method that tells you which flags were used, precisely to avoid these potential conflicts. A better named Collection<String> usedFlags(), on CommandContext, if you will.

[WillBAnders]

I like that solution better than merging it into arguments because it's effectively not an argument. A Collection<String> would make it difficult to lookup individual flags though, a map seems more appropriate there. What about adding a hasFlag(String) method that falls back to an internal map?

[dualspiral]

Honestly, having slept on it, I'm not sure it's necessary at all. Or rather, I can't really see a use case for this information precisely because they aren't arguments and they should have been handled by being given a value, or whatever.

My thought about a collection is that we would just need to store the names of the flags. Then you can get a copy of that collection and inspect it as will. I don't see it needing to be any more complex than that - I'm not sure exactly why a map is necessary but, hey, the devil will be in the detail, I'm sure.

If you make a PR and explain what use cases there are, I'm not against it. I'm just not sure there is any benefit so I'd like to see what they are.

[ImMorpheus]

PR merged