Argu icon indicating copy to clipboard operation
Argu copied to clipboard

ArgumentParser<_>.PrintCommandLineArgumentsFlat does not escape '\n'

Open kleidemos opened this issue 6 years ago • 5 comments

Repro steps

module BugInArgu =
  open Expecto
  open Expecto.Flip
  open Argu

  type Args =
    | SomeString of string

    interface IArgParserTemplate with
      member this.Usage =
        match this with
        | SomeString _ -> "NIE"

  let parser =
    ArgumentParser<Args>()

  [<Tests>]
  let tests =
    ftestList "argu" [
      testCase "string contains new line" <| fun () ->
        let str = "\nnewLine\nnewLine\n"
        let args = [
          SomeString str
        ]
        
        args
        |> parser.PrintCommandLineArgumentsFlat
        |> Expect.equal "" (sprintf "--somestring %A" str)
    ]

Actual behavior

[23:40:21 ERR] argu/string contains new line failed in 00:00:00.0420000.
. String does not match at position 13. Expected char: '"', but got '\010'.
expected:
--somestring "
newLine
newLine
"
  actual:
--somestring
newLine
newLine

   at...

kleidemos avatar Sep 08 '19 19:09 kleidemos

I'm not sure I understand what the expected behaviour is. The method prints literal quotes but fails to do so if you add newline chars?

eiriktsarpalis avatar Sep 16 '19 08:09 eiriktsarpalis

PrintCommandLineArgumentsFlat must quote parameters with newline chars, like whitespaces.

kleidemos avatar Sep 17 '19 14:09 kleidemos

PrintCommandLineArgumentsFlat must quote parameters with newline chars, like whitespaces.

Could you elaborate why? Why shouldn't it be quoted if that is not the case?

eiriktsarpalis avatar Sep 17 '19 15:09 eiriktsarpalis

Sorry. But I didn't understand or couldn't translate the question.

  • Using %A generates the required string only in this case. I was too lazy to write full implementation.
  • We can quote any string, even if it is not necessary. But it doesn't seem good.

kleidemos avatar Sep 18 '19 12:09 kleidemos

@kleidemos Would you be able to provide a PR? Eirik is no longer maintaining this as of #191

Realistically the fact that this API is not a mainstream thing means resolving this won't get to the top of the up for grabs or good-first-issue lists in any reasonable length of time (I'm trying to make this issues list tractable, as no issue will ever get resolved if its just a dumping ground)

bartelink avatar Dec 12 '23 13:12 bartelink