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

Consider Access to the source input #16

Open
AngelMunoz opened this issue Apr 12, 2023 · 4 comments
Open

Consider Access to the source input #16

AngelMunoz opened this issue Apr 12, 2023 · 4 comments

Comments

@AngelMunoz
Copy link
Contributor

Hello Jordan 👋 here I am once again!

I was trying to add tests for my commands and I figure out that one may want to access the source input to do multiple kind of operations on them given that the SLC library seems to use the inputs as contextual information to to either request values or to access the machinery under it

After looking at your code I came up with this:

[<AutoOpen>]
module Extensions =

  type HandlerInput<'a> with
    
    // I think these two could be good additions but I'm not
    member this.GetArgument<'T>(): Argument<'T> option =
      match this.Source with
      | ParsedArgument a -> a :?> Argument<'T> |> Some
      | _ -> None

    member this.GetOption<'T>(): Option<'T> option =
      match this.Source with
      | ParsedOption o -> o :?> Option<'T> |> Some
      | _ -> None

    // This is like the one you already have but with ParseResult rather than invocation context
    member this.GetValue(ctx: ParseResult): 'T option =
      match this.Source with
      | ParsedOption o -> o :?> Option<'T> |> ctx.GetValueForOption |> Option.ofNull 
      | ParsedArgument a -> a :?> Argument<'T> |> ctx.GetValueForArgument |> Option.ofNull
      | Context -> None

Note: Option.ofNull comes from FsToolkit.ErrorHandling

And I can use it like this:

module CommandOptions =

  [<Fact>]
  let ``Commands.Setup can parse options`` () =
    let root = RootCommand("Commands.Setup can parse options")
    let skipPrompts = SetupInputs.skipPrompts
    let skipPlaywright = SetupInputs.skipPlaywright

    root.AddCommand(Commands.Setup)
    let result = root.Parse([| "setup"; "-y"; "--skip-playwright"; "false" |])

    Assert.Empty(result.Errors)

    let skipPlaywright: bool option = skipPlaywright.GetValue result |> Option.flatten
    let skipPrompts: bool option = skipPrompts.GetValue result |> Option.flatten

    Assert.True (skipPrompts |> Option.defaultValue false)
    Assert.False (skipPlaywright |> Option.defaultValue true)

In this case I only have access to the ParseResult but to be able to get the value I need access to the source, thankfully you have already a similar function, but it requires the invocation context rather than the parsing results.

For my purposes I think this is just what I need but I'll leave it here in case this seems useful
Feel free to discuss/close as this is not a feature request

Note: I'm aware of your testRootCommand builder in the tests project, would it be worth considering adding it as a test helper package?

@JordanMarr
Copy link
Owner

JordanMarr commented Apr 12, 2023

Having the testRootCommand available to the user is a good idea.

I agree that users should never have to unwrap the HandlerInput type manually, so I'm all for having the necessary helper methods to allow them to extract what they want. Is there a specific example of where a user might need to extract the underlying Option<'T> / Argument<'T>? I'd rather not add anything extra unless there is a specific use case -- mostly because I'm not sure how the underlying library may evolve through its ongoing beta iterations.

Regarding the GetValue method:
Would a user ever call GetValue on an InvocationContext input? I feel like that is unlikely, so this would make it easier to consume the value without have to unwrap an option type:

    member this.GetValue(parseResult: System.CommandLine.Parsing.ParseResult) =
        match this.Source with
        | ParsedOption o -> o :?> Option<'T> |> parseResult.GetValueForOption
        | ParsedArgument a -> a :?> Argument<'T> |> parseResult.GetValueForArgument
        | Context -> failwith "Cannot get value from InvcationContext input source."

@JordanMarr
Copy link
Owner

I was just checking out your Perla wizard code and I glanced at the way you are organizing your commands and HandlerInputs into modules. Looks interesting. At some point I need to pull your code down and analyze they way you are organizing it so I can have a better perspective of what's good or bad about FS.SCL in the context of a large project with logs of commands/inputs.

@AngelMunoz
Copy link
Contributor Author

Is there a specific example of where a user might need to extract the underlying Option<'T> / Argument<'T>? I'd rather not add anything extra unless there is a specific use case -- mostly because I'm not sure how the underlying library may evolve through its ongoing beta iterations.

I was iterating on what would be a good setup to test my commands but when I started looking into the SCL library itself it seems that if you want to add typed handlers you need to have access to the options/arguments e.g.

let a = Argument(...)
let b = Option(...)

let command1 = Command()
command1.AddOption(a)
command1.AddArgument(b)

// here you need to be able to point at the options/arguments
// which we may not care at all that's what your project is about not having
// to deal with this
command1.SetHandler(Func<_,_, int>(fun a b -> 0), a, b)

let result = command1.Parse("the thing --and-that")

result. // <- at this point is when things get tricky
// and you need to have either the option/argument around to do anything meaningful

I agree with you that there are not many cases where one may need to access the option/argument outside testings that's what is driving my extension here, for the moment

This is what my tests are starting to look like and I think it's good enough for my case:

let GetRootCommand (handler: Command, command: string) =
    let root = RootCommand()
    root.AddCommand(handler)
    root.Parse(command)

  [<Fact>]
  let ``Commands.Setup can parse options`` () =
    let result =
      GetRootCommand(Commands.Setup, "setup -y --skip-playwright false")

    let skipPlaywright: bool option =
      // with your updated extension to avoid wrapping options within options
      SetupInputs.skipPlaywright.GetValue result

    let skipPrompts: bool option =
      SetupInputs.skipPrompts.GetValue result

    Assert.Empty(result.Errors)
    Assert.True(skipPrompts |> Option.defaultValue false)
    Assert.False(skipPlaywright |> Option.defaultValue true)

For the moment I don't care too much about if the execution is well tested, but I do want to be sure that my custom inputs are parsing values correctly as I've run into some bug issues myself when testing the CLI app.

So if I had to distil a core need here would be...

  • Being able to easily test argument/option parsing and values when it has been integrated with a root command
  • Being able to test if a handler did receive the correct inputs from different cli string "commands"

At some point I need to pull your code down and analyze the way you are organizing it so I can have a better perspective of what's good or bad about FS.SCL in the context of a large project with logs of commands/inputs.

Ahh! I recently merged in dev my current approach to this so feel free to take a look at the dev branch. Don't pay too much attention to the logging stuff as I think I screwed up there, but I think command related things... are good enough for my needs.

@JordanMarr
Copy link
Owner

I like the way your wizard prefixes the output with a prefix (Perla:, Esbuild, etc).
I was just noticing in the SqlHydra wizard that my > prefix for my prompts could be improved but I couldn't think of how.
It would probably be easier to read if did the same and used SqlHydra:.

Probably good that your wizard needs to be explicitly started by calling the setup command.
In SqlHydra, it detects if a config file has been created yet and starts the wizard if not. It's fine, but sometimes I do wonder if I should have made it more explicit.

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

No branches or pull requests

2 participants