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

Command line argument parsing improvements #1048

Conversation

FrankRay78
Copy link
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
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!

Copy link
Contributor

@patriksvensson patriksvensson left a comment

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?

@nils-a
Copy link
Contributor

nils-a commented Dec 1, 2022

Anything you would want to be addressed before merging?

Nothing that you haven't addressed already. 👍

@patriksvensson
Copy link
Contributor

patriksvensson commented Dec 4, 2022

@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 force-pushed the PR-193-587-959-Command-line-argument-parsing branch from bf32bb5 to bc341f7 Compare December 4, 2022 21:00
@FrankRay78
Copy link
Contributor Author

FrankRay78 commented Dec 4, 2022

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

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

@FrankRay78 Merged!

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

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