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

GH-587: Negative integer short values #1040

Conversation

FrankRay78
Copy link
Contributor

This is a rebased version of an existing PR, namely #732 ("Fixes #587 Support negative numbers as command option values") which was created on 16 Feb 2022 and is now 94 commits behind main.

Some of the conflicts arising from the rebase required manual fixing as directories had been deleted / relocated.

I am submitting this updated PR directly here (and not in addition to the original PR) because I am not authorised to push to jouniheikniemi's original branch.

Additionally, I've addressed @phil-scott-78 's previous review comment:

But think we could get some tests that cover the invalid cases as described in the comments of the tokenizer for invalid items?

And made some minor stylistic updates of my own to largely keep the code base consistent, arising from my review comments I left on the earlier PR.

@FrankRay78
Copy link
Contributor Author

FrankRay78 commented Nov 1, 2022

ℹ️ Note ℹ️ - 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.

I plan to merge all three PR's onto a single local branch to examine the extent of the conflicts, and hopefully resolve them as I go. It might be prudent to consider waiting and then merging this resolved, single branch @patriksvensson, rather than the 3 separate PR's mentioned above.

@patriksvensson
Copy link
Contributor

@FrankRay78 Whatever works best for you! Planning on sitting down this week and look through your work. A lot of nice changes!

@FrankRay78
Copy link
Contributor Author

@FrankRay78 Whatever works best for you! Planning on sitting down this week and look through your work. A lot of nice changes!

I recommend waiting for the consolidated branch - I'm right in the middle of merging it now and it will be done in the next day or two. I'm also squashing the various commits in each previous PR so it's easier to see the changes. It's taken me a little while to get to grips with git properly, apologies for the amateur PR's previously...

@patriksvensson patriksvensson changed the title 587 negative integer short values rebased 20221101 GH-587: Negative integer short values Nov 4, 2022
@FrankRay78
Copy link
Contributor Author

Closing as superseded by #1048

@FrankRay78 FrankRay78 closed this Nov 5, 2022
@FrankRay78 FrankRay78 deleted the 587-negative-integer-short-values-REBASED-20221101 branch November 9, 2022 13: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.

Cannot specify negative number as command option value
2 participants