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

Better name based lookup #2391

Open
KathleenDollard opened this issue Apr 22, 2024 · 0 comments
Open

Better name based lookup #2391

KathleenDollard opened this issue Apr 22, 2024 · 0 comments
Labels
Powderhouse Work to isolate parser and features

Comments

@KathleenDollard
Copy link
Contributor

Based on conversations with @jonsequitur, I think we can improve name based lookups in Powderhouse.

Problem

e.g. consider this command line:

myapp --opt1 123 subcommand --opt1 "hello"

If --opt1 is a different symbol then name-based lookup has to make some assumptions that I think lead to some nasty bugs.

Let's assume the first instance is a CliOption and the second is CliOption, I can't write code like this:

var value = parseResult.GetValue<int>("--opt1");

It will behave differently (and in cases throw) based on the command line we're parsing.

The reason the option at an earlier position is sometimes desired is because handlers for multiple subcommands might want to reference that value.

So now common or cross-cutting code can't make a strong assumption that the name-based lookup will always return the expected type to match the generic parameter.

This is why I said this API is ergonomically appealing but has some correctness issues.

Proposed solution

Instead of building the dictionary for the full tree root down, build it to include only the ancestors of the current command.

This is a breaking change in that you can't get default values for cousins by just saying GetValue. I think that is justified because the user did not enter those values.

Rather than put the name lookup dictionary on SymbolResultTree, put it on CommandResult. The logic will be that the CurrentCommandResult is checked first, where it will generally be found. If it is not found, or the type is wrong, check up the ancestor tree.

We will allocate another dictionary or two, but the dictionaries will be smaller as the whole tree won't be traversed, just the CurrentCommand and its ancestors.

This will also allow a different scenario. If both of the --opt1 options are CliOption<string> and the user wants to get the one on an ancestor. If we decide this is a valid scenario (and it looks like one to me), we can add an API where the user includes the name or symbol for the ancestor command, and we start the search there.

@KathleenDollard KathleenDollard added the Powderhouse Work to isolate parser and features label Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Powderhouse Work to isolate parser and features
Projects
None yet
Development

No branches or pull requests

1 participant