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

Added post processing checking of sub command mandatory flag #127

Merged
merged 5 commits into from
Dec 7, 2020
Merged

Added post processing checking of sub command mandatory flag #127

merged 5 commits into from
Dec 7, 2020

Conversation

chestercodes
Copy link
Contributor

I think that this fixes - #116 ?
I'm new to the project (and fairly new to F#) so I appreciate I might have missed something in the implementation.

@@ -66,7 +66,23 @@ let postProcessResults (argInfo : UnionArgInfo) (ignoreMissingMandatory : bool)
| Choice1Of2 ts, ts' when caseInfo.GatherAllSources.Value -> Array.append ts ts'
| _, ts' -> ts'

let rec searchSub caseInfo =
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure this could be done in a less allocatey way, since you're only really looking for the first element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that better? the tests pass. Is the wilcard return empty array the right call on line 78?

I'm not 100% sure I understand why unionArg.Cases.Value is a single item array. Is it because any duplicates will have been filtered out by the time this runs?

Copy link
Member

Choose a reason for hiding this comment

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

Since the only searchSub callsite only checks for the first array element, it should really just return an optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it to an option

@eiriktsarpalis
Copy link
Member

Apologies, this PR seems to have slipped through the cracks. I've rebased your changes on top of the latest master.

@eiriktsarpalis
Copy link
Member

Thank you!

@eiriktsarpalis eiriktsarpalis merged commit 9a5c64f into fsprojects:master Dec 7, 2020
@simonjpascoe
Copy link

simonjpascoe commented Feb 13, 2021

Can we get a release pushed with this patch in please?

@Angr1st
Copy link

Angr1st commented Nov 8, 2021

The more I test this the more I think this does not work properly at all. The actual parsed result is never checked rather it is only looked at the unioncase definition which doesn't make any sense to me because that means that even when a value is provided this will fail. Also it can currently only work for single case unions as a result it does not fix #116 .
See this test case:

[<Fact>]
let ``Main command parsing with mandatory sub command's sub command parameter`` () =
    let args = [|"--mandatory-arg"; "true"; "tag"; "--new"; "--name"; "test"|]
    let results = parser.ParseCommandLine args
    let nested = results.GetResult <@ Tag @>
    let nested' = nested.GetResult <@ New @>
    test <@ nested'.Contains Name @>

@eiriktsarpalis is there an easy way to get the parsed value ({Argu.UnionArgInfo.UnionParseResults}) during the post processing stage to enable checking if it was provided? During debugging it seems to be possible but at design-time everything is obj. At runtime the objects seem deeply nested which would require a lot of dynamic casting to get to the actual value.

@mrboring
Copy link

As @Angr1st has indicated, this code does not work as expected. I got the following results:

  1. No --name parameter. The expected error is generated:
argv: [|"--mandatory-arg"; "true"; "tag"; "--new"|]
ERROR: missing parameter '--name'.
USAGE: gadget tag --new [--help] --name <string>

OPTIONS:

    --name <string>       New name
    --verbose, -v         be verbose.
    --data <int> <base64> pass raw data in base64 format.
    --help                display this list of options.
  1. No value for the --name parameter. The expected error is generated:
argv: [|"--mandatory-arg"; "true"; "tag"; "--new"; "--name"|]
ERROR: argument '--name' must be followed by <string>.
USAGE: gadget tag --new [--help] --name <string>

OPTIONS:

    --name <string>       New name
    --verbose, -v         be verbose.
    --data <int> <base64> pass raw data in base64 format.
    --help                display this list of options.
  1. The --name parameter exists with a valid value. There should be no error, but one occurs:
argv: [|"--mandatory-arg"; "true"; "tag"; "--new"; "--name"; "myName"|]
ERROR: missing parameter '--name'.
USAGE: gadget tag --new [--help] --name <string>

OPTIONS:

    --name <string>       New name
    --verbose, -v         be verbose.
    --data <int> <base64> pass raw data in base64 format.
    --help                display this list of options.

I've spent some time looking through the code but can't work out how to fix it.

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.

5 participants