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

439 expand dynamic arg expands non dynamic args #440

Merged

Conversation

npetzall
Copy link
Contributor

fixes issue #439

When looking the names loop wasn't duplicated in any validating fashion.
So adding a handlesArg method wouldn't improve anything.
Extracting a method might have been nice, but it would require three arguments.

Copy link
Collaborator

@mkarg mkarg left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. Unfortunately your solution is removing an existing feature. Can you please modify it?

Update: Nonsene. I did not read the original bug description carefully enough.

@npetzall npetzall force-pushed the 439_expandDynamicArg_expands_nonDynamicArgs branch from be2bc40 to 27de115 Compare December 21, 2023 16:03
@mkarg
Copy link
Collaborator

mkarg commented Dec 21, 2023

Nils, you force-pushed, but the result still is the same. Is this by intention?

@npetzall
Copy link
Contributor Author

npetzall commented Dec 21, 2023

Nils, you force-pushed, but the result still is the same. Is this by intention?

I did so because I'm 5 years after master, so need to get all changes in.

@mkarg
Copy link
Collaborator

mkarg commented Dec 22, 2023

Nils, you force-pushed, but the result still is the same. Is this by intention?

I did so because I'm 5 years after master, so need to get all changes in.

That's fine, but do you also plan to fix the open issue I raised yesterday?

@npetzall
Copy link
Contributor Author

@mkarg I would gladly solve it, if you could provide me with a way of testing the existing feature so that I can make sure that I'm not breaking it.

@mkarg mkarg merged commit eaac195 into cbeust:master Dec 22, 2023
1 check passed
@mkarg
Copy link
Collaborator

mkarg commented Dec 22, 2023

Sorry for the delay and thank you very much for your contribution to JCommander, Nils! :-)

@mkarg mkarg added the bug Bug label Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants