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.

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

Add defined flags to CommandContext #2033

Merged
merged 3 commits into from Jul 25, 2020

Conversation

WillBAnders
Copy link
Contributor

Overview

Based on discussion in #1978, being able to differentiate between an absent flag and a flag with an absent optional value could be useful for handling default values. Additionally, this is extremely helpful when working with complex flag systems where the same value could be set through multiple flags (used in validation). The PR is intended to provide a base implementation and serve as discussion for whether this is worth adding to the API.

Additionally, given the new hasFlag method it would make sense to have this be the recommend way for checking non-value flags.

Implementation

This adds methods to CommandContext for defining and checking flags, implemented with a set. Methods that accept a Text are also provided to match existing putArg methods, but this may not be necessary.

To handle parsing, a new FlagElement has been added that ensures flags are added to the context. The key used is the first flag found, same as for the element used previously. Notably, this replaces the use case for the MarkTrue element used previously, so this could potentially be deprecated as well.

Additionally, I made the decision to also define the flag for the ACCEPT_VALUE and ACCEPT_NONVALUE behaviors for unknown flags. This is probably worth discussing as well.

Copy link
Contributor

@dualspiral dualspiral left a comment

Choose a reason for hiding this comment

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

I think this needs to change as suggested in the comments.

I'm still not sure about whether we need this. A well built command should make it simple for the consumer to know which value comes from where. If the changes are minimal and purely additive, then there isn't really a reason to reject it outright either, sans that it might be seen as bloat.

Please note that as the API 8 command system stands, we don't have flags. That's not for lack of trying, I'm trying to figure out the best way to re-add them, but the client completion stuff is actually making it really difficult. They will likely become "anchor flags", at least at first.

Just be aware of that.

… and delegate to valueElement for completion when present.
@dualspiral dualspiral added this to PRs: Held in 7.x LTS Issue Triage Jun 9, 2020
@Zidane
Copy link
Member

Zidane commented Jun 28, 2020

@dualspiral Now or never on this for API-7. If not close it.

@dualspiral
Copy link
Contributor

I'll test, it's my fault it's been held and I think it would be unfair to close it. Will decide based on that.

Copy link
Contributor

@dualspiral dualspiral left a comment

Choose a reason for hiding this comment

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

I think I owe you an apology @WillBAnders. After working on the new API 8 system, parsed flags are indeed separate from the context. I know I argued against it but... it made sense in the new system and so it deserves to be here. My apologies for running you around the houses.

This looks okay as is, one minor comment. If you don't want to do it, let me know and I'll get around to doing so.

@dualspiral dualspiral merged commit 24c2f01 into SpongePowered:stable-7 Jul 25, 2020
7.x LTS Issue Triage automation moved this from PRs: Held to Complete Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants