Skip to content

Command line argument parsing improvements#1048

Merged
patriksvensson merged 4 commits into
spectreconsole:mainfrom
FrankRay78:PR-193-587-959-Command-line-argument-parsing
Dec 5, 2022
Merged

Command line argument parsing improvements#1048
patriksvensson merged 4 commits into
spectreconsole:mainfrom
FrankRay78:PR-193-587-959-Command-line-argument-parsing

Conversation

@FrankRay78

Copy link
Copy Markdown
Contributor

The following three PR's, when taken jointly, represent a significant enhancement to existing command line parsing behaviour: #1036, #1029, #1040

However, they all make changes to CommandTreeTokenizer and introduce their own unit tests specifically for the CommandTreeTokenizer, and as such, merge conflicts will arise once any one of the PR's has been merged.

This PR encompasses all three PR's, successfully merged and with all conflicts resolved. The respective git histories have also been squashed so the changes for each issue can be more clearly examined.

All issues addressed by this single combined PR are: #959, #842, #890, #193 (and its open duplicate, #825), #587

As it's quite a big change, I'm happy to help in any way required @patriksvensson for you to accept in full, in part, with or without amendment etc.

@FrankRay78 FrankRay78 changed the title PR 193 587 959 command line argument parsing Issues 193 587 959 command line argument parsing Nov 3, 2022
@patriksvensson patriksvensson changed the title Issues 193 587 959 command line argument parsing GH-193, GH-587, GH-959: Command line argument parsing Nov 4, 2022
@patriksvensson patriksvensson changed the title GH-193, GH-587, GH-959: Command line argument parsing Command line argument parsing improvements Nov 4, 2022
@patriksvensson

Copy link
Copy Markdown
Contributor

@FrankRay78 From a quick look, it looks ok to me! I will set aside some time this weekend to dig into it a bit more.

Does this one cover other pull requests? If so, could you close the other ones to avoid confusion?

Thanks!

@patriksvensson patriksvensson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A minor thing, but otherwise, it looks good to me 👍

@phil-scott-78, @nils-a Anything you would want to be addressed before merging?

Comment thread src/Spectre.Console.Cli/Internal/Parsing/CommandTreeTokenizer.cs Outdated
@nils-a

nils-a commented Dec 1, 2022

Copy link
Copy Markdown
Contributor

Anything you would want to be addressed before merging?

Nothing that you haven't addressed already. 👍

@patriksvensson

patriksvensson commented Dec 4, 2022

Copy link
Copy Markdown
Contributor

@FrankRay78 There are currently conflicts with the main branch. Could you rebase your PR branch with an updated main branch?

If you're uncertain how to do this, we've got a guide for it in our Contribution Guidelines.

I can also help you out with this if you add me as a contributor to your Spectre.Console repository.

@FrankRay78

FrankRay78 commented Dec 4, 2022

Copy link
Copy Markdown
Contributor Author

I've rebased and fixed the merge conflicts @patriksvensson.

@patriksvensson patriksvensson merged commit b793482 into spectreconsole:main Dec 5, 2022
@patriksvensson

Copy link
Copy Markdown
Contributor

@FrankRay78 Merged!

Thank you for this. Stellar work! Really happy with your work! 👍

@FrankRay78 FrankRay78 self-assigned this Dec 11, 2022
@FrankRay78 FrankRay78 deleted the PR-193-587-959-Command-line-argument-parsing branch December 16, 2022 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment