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

Suggestion: Docopt.Parse overload that takes list rather than array #2433

Closed
rmunn opened this issue Nov 29, 2019 · 7 comments · Fixed by #2525
Closed

Suggestion: Docopt.Parse overload that takes list rather than array #2433

rmunn opened this issue Nov 29, 2019 · 7 comments · Fixed by #2525

Comments

@rmunn
Copy link
Contributor

rmunn commented Nov 29, 2019

Description

The Docopt.Parse method in Fake.Core.CommandLineParsing takes a string array as parameter, which is perfect for handling arguments retrieved via Target.getArguments(). But when I experimented with getting the arguments out of the TargetContext, where they're stored as a string list, I had to write parser.Parse(p.Context.Arguments |> Array.ofList) instead of parser.Parse(p.Context.Arguments). It's just a trivial annoyance, but I would have liked to see a parser.Parse overload that takes a string list. (Since it's a class method, overloading is possible and should be pretty trivial in this case).

Expected behavior

I want to be able to write:

Target "Foo" (fun p ->
    let usage = """
Usage:
  Foo [options]

Options:
  -v, --verbose  Verbose
"""
    let parser = Docopt(usage)
    let parsedArgs = parser.Parse(p.Context.Arguments)
    if parsedArgs |> DocoptResult.hasFlag "-v" then
        Trace.tracefn "Would be verbose"
)

Actual behavior

I currently have to write this instead:

    let parsedArgs = parser.Parse(p.Context.Arguments |> Array.ofList)

Notes

I know the FAKE docs currently say "Using the current parameter object is not supported yet" because APIs might change. But that actually makes me think of another possible solution, which would be to change the API of TargetContext. Specifically, make the TargetContext.Arguments type to be a string array instead of a string list. Now, that's more complicated than my proposed overload to parser.Parse, which would only add a single line of code. But it might be a simplification; as I looked through the FAKE code for uses of TargetContext.Arguments that might have to change, I saw a lot of List.toArray calls, so perhaps it would be simpler all around for TargetContext.Arguments to be a string array. Either way would make me happy. (And honestly, I wouldn't be unhappy if it was left as-is; it's a trivial thing to add a List.toArray or Array.ofList call when I need it. It's just a minor bit of friction that could be removed, that's all.)

Related information

  • Operating system: any
  • Branch: N/A (I'm using released FAKE from NuGet)
  • .NET Runtime, CoreCLR or Mono Version: any (I'm using .Net Core 3.0)
  • Indications of severity: trivial
  • Version of FAKE (4.X, 5.X): 5.18.3
@matthid
Copy link
Member

matthid commented Nov 29, 2019

I think we could take this low-risk improvement. Is there any reason to not just add an IEnumerable/seq based overload?

Regarding the docs: From the beginning I think it was not the best idea to only have the arguments available in the target callback instead of the script context.
Now we have the required API for this if you call let args = Target.getArguments() at the top of your script. see https://fake.build/core-targets.html

@github-actions
Copy link
Contributor

There has not been any activity in this issue for the last 3 months so it will be closed in 14 days if there is no activity.

@github-actions github-actions bot added the stale label Feb 28, 2020
@rmunn
Copy link
Contributor Author

rmunn commented Feb 28, 2020

A seq-based overload would be the simplest from an API use point of view; I'll plan to do that. (I'd forgotten about this issue, which is why I haven't done that yet. PR coming soonish.)

@rmunn
Copy link
Contributor Author

rmunn commented Mar 14, 2020

Still haven't forgotten about this, but I guess I didn't act on it fast enough for the bot. Reopen please, so I don't lose track of this?

@matthid matthid removed the stale label Mar 14, 2020
@matthid
Copy link
Member

matthid commented Mar 14, 2020

Closing this doesn't mean that a pull request is no longer welcomed. Quite the reverse: Feel free to send a pull request any time.
I added the bot in order to keep the issue count manageable and remind myself (and others) of issues.
I can of course increase the period, but when nothing happened for 3 months (and then within 15 days of the remainder) I felt like this is a good indicator to just close it. Or at least question the value of keeping the issue open.
Of course I can reopen and it will restart the process, on the other hand you can set a reminder in your calendar in 30 days which might have the same effect :)

@matthid matthid reopened this Mar 14, 2020
@github-actions
Copy link
Contributor

There has not been any activity in this issue for the last 3 months so it will be closed in 14 days if there is no activity.

@github-actions github-actions bot added the stale label Jun 13, 2020
@rmunn
Copy link
Contributor Author

rmunn commented Jun 24, 2020

Starting work on this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants