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

WIP: Separate args for each command #551

Merged
merged 9 commits into from
Jan 19, 2015

Conversation

theimowski
Copy link
Member

Because Paket.exe is always to be invoked with a given command, I thought it would be nice to separate arguments for each of the commands.

  • clearer view of which command uses what arguments
  • easier to express the syntax of commands (see f.e issue in WIP - paket add support for HTTP, gist and github dependencies #511)
  • good way to maintain the command line help (will need to fill in the usage for arguments)
  • potential for automated (build process) command syntax in markdown docs

Tell me what you think, if you give green light I'll proceed with filling up the usages, and what else gives.

| Some x -> x
| _ -> ""
match args with
| "add" :: args ->
Copy link
Member

Choose a reason for hiding this comment

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

why did we loose the nice discriminated union case here?
@mexx what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Actually neither version looks good to me.
The old one do not support our use case for having separate argument for each command well.
The new one tries to provide the desired behavior but introduces redundancy and magic values.

I have to look further into Nessos.UnionArgParser to see how we can do better here.
I'll try to look at it on this weekend.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm I'll see what I can do to bring the Commands DU back. @mexx what type of redundancy? You mean arguments that are applicable to more than one command?

@forki
Copy link
Member

forki commented Jan 14, 2015

Yes I think making the args more explicit per command is a good thing.

@theimowski theimowski force-pushed the separate_args_for_each_command branch from 9e14d29 to a7c5685 Compare January 15, 2015 15:17
@theimowski
Copy link
Member Author

I added the Command DU - does it look any better now?


if results.IsUsageRequested then
trace <| results.Usage("paket config")
Copy link
Member

Choose a reason for hiding this comment

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

Why not showHelp HelpTexts.commands.["config"]?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no "config" in this dictionary

Copy link
Member

Choose a reason for hiding this comment

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

It was the actual implementation. Congratulations, you found a bug ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, indeed, didn't notice that one ;)

@mexx
Copy link
Member

mexx commented Jan 18, 2015

Looks a little bit better now. With this version of UnionArgParser it seems to be the best achievable result, albeit the filterGlobalArgs looks like a hack with this magic strings in it.

forki added a commit that referenced this pull request Jan 19, 2015
@forki forki merged commit 26f246a into fsprojects:master Jan 19, 2015
@theimowski theimowski deleted the separate_args_for_each_command branch January 19, 2015 09:26
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.

3 participants