cannorin / FSharp.CommandLine

A framework for building command line application in F#
Apache License 2.0
57 stars 1 forks source link

Bug with the dig function #7

Open mac10688 opened 3 years ago

mac10688 commented 3 years ago
  let rec private dig argv (cmd: Command<int>) =
    let config = (cmd.config CommandInfo.empty)
    match argv with
      | [] -> (cmd, [])
      | args ->
        match config.options |> List.choose (fun o -> o.isMatch args)
                             |> List.tryHead with
          | Some rest -> dig rest cmd
          | None ->
            match config.subcommands |> List.tryFind (fun sc -> sc.Summary().name = List.head args) with
              | Some sc -> dig (List.tail args) sc
              | None -> (cmd, args)

I think

        match config.options |> List.choose (fun o -> o.isMatch args)
                             |> List.tryHead with
          | Some rest -> dig rest cmd

will be the cause of infinite recursion.

Really I am trying to write a unit test for the generator function, which calls the dig method and I'm trying to rationalize what it's supposed to do. I don't understand how isMatch is supposed to work. It sounds like something that would return a boolean but instead it returns an option of list of strings?

Here is my first pass at seeing how the generator function works and it hit infinite recursion.

[<Test>]
let generator () =

    let commandSummary  = {
        name = "Name"
        displayName = Some "DisplayName"
        description = "Test description"
        paramNames = None
        help = None
        genSuggestions = fun _ -> []
    }

    let commandOption1 = {
        names = ["commandOptionName1"]
        description = "description"
        isFlag = false
        paramNames = [["paramNames1"]]
        isMatch = fun _ -> Some ["Matches"]
        genSuggestions = fun _ -> []
    }

    let commandOption2 = {
        names = ["commandOptionName2"]
        description = "description"
        isFlag = false
        paramNames = [["paramNames2"]]
        isMatch = fun _ -> Some ["Matches"]
        genSuggestions = fun _ -> []
    }

    let command1 = {
        config = fun _ -> {
            summary = {
                name = "SubName1"
                displayName = Some "SubDisplayName1"
                description = "Sub Test description 1"
                paramNames = Some ["SubTest1"; "SubParams1"]
                help = None
                genSuggestions = fun _ -> []
            }
            options = []
            subcommands = []
        }
        func = fun args -> (0, args)
    }

    let command2 = {
        config = fun _ -> {
            summary = {
                name = "SubName2"
                displayName = Some "SubDisplayName2"
                description = "Sub Test description 2"
                paramNames = Some ["SubTest2"; "SubParams2"]
                help = None
                genSuggestions = fun _ -> []
            }
            options = []
            subcommands = []
        }
        func = fun args -> (0, args)
    }

    let commandInfo = {
        summary = commandSummary
        options = [commandOption1; commandOption2]
        subcommands = [command1; command2]
    }

    let cmd = {
        config = fun _ -> commandInfo
        func = fun args -> (5, args)
    }

    let x = Generators.Help.generate [""] cmd 
    x |> should equal [""]
cannorin commented 3 years ago

Nice catch.

isMatch (I don't remember why I used such a bad name!) is supposed to take a list of arguments and return Some restOfTheArguments if the option matches the input (with the restOfTheArguments containing the arguments the option does not use), or None otherwise. The basic idea here is that the list of arguments should reduce its length when an option matches.

I assumed it is always implemented here: https://github.com/cannorin/FSharp.CommandLine/blob/master/src/FSharp.CommandLine/options.fs#L43-L52

And since ICommandOption><_>.Config always use the above implementation: https://github.com/cannorin/FSharp.CommandLine/blob/master/src/FSharp.CommandLine/options.fs#L57-L61, there should be no chance that a hand-written isMatch would be used.

Here, the part isMatch = fun _ -> Some ["Matches"] is causing the infinite loop, since the argument list would always be ["Matches"].

Do you feel this library should be heavily refactored? I'm actually thinking of it.

mac10688 commented 3 years ago

I would like to fully understand the code base and what it's capable of before I judge. That's why I started writing these unit tests, because reading through it I couldn't understand anything and as you mentioned elsewhere it could use a lot more documentation.

It may take me a few months to finish my unit tests to create my PR. After I finish my unit tests, I'll take the opportunity to judge the framework and think of suggestions.

Overall though, I think you have a good vision for what needs to happen to make it better and I'm hoping I can help anyway I can.