Skip to content

use Validator to set Handler for VersionOption #2042

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

Closed
wants to merge 1 commit into from

Conversation

adamsitnik
Copy link
Member

@jonsequitur while reviewing #1969 I reminded myself about the concept of ActiveOption and got to conclusion that it can be implemented by using existing public APIs.

During validaiton of an OptionResult we know that it's Parent must be CommandResult. CommandResult exposes Command, which exposes Handler. So we can set it during validation.

In case of VersionOption we can validate that there are no errors and then just set the handler.

I am not sure whether Validators should have side effects like this, I just want to hear your thoughts on this.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Feb 2, 2023

That's going to blow up if an interactive application has a static Command tree and uses that to parse multiple argument lists and invoke the commands. One of the argument lists includes --version and causes the validator to run and change the handler of the Command, and then future argument lists can no longer invoke the original handler, even if they do not include --version.

Even worse if it's a service that parses argument lists and invokes commands in parallel threads.

@jonsequitur
Copy link
Contributor

I agree with @KalleOlaviNiemitalo.

As a principle, we should avoid validators (or anything that happens during parsing) creating side effects.

Another consideration to keep in mind is that analysis of the parser configuration, without parsing things, should be able to provide information such as which options are "active" / invokable.

@KalleOlaviNiemitalo
Copy link

reminded myself about the concept of ActiveOption

What concept is that?

@adamsitnik adamsitnik closed this Feb 6, 2023
@adamsitnik
Copy link
Member Author

What concept is that?

An option which performs an action when it's provided by the user. Examples: --help (displays helps) and --version (displays version)

@adamsitnik adamsitnik deleted the activeOptionValidator branch February 6, 2023 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants