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

Allow aliases of a CommandSpec that is already a subcommand to be properly & consistently modified. #1529

Merged
merged 4 commits into from
Feb 7, 2022
Merged

Allow aliases of a CommandSpec that is already a subcommand to be properly & consistently modified. #1529

merged 4 commits into from
Feb 7, 2022

Conversation

rgoldberg
Copy link
Contributor

@rgoldberg rgoldberg commented Dec 23, 2021

Allow aliases of a CommandSpec that is already a subcommand to be properly & consistently modified.

Resolves #1528

Doesn't yet contain tests. Wanted feedback before I implement tests.

Will add tests and whatever else is requested once the current state of the PR has been reviewed.

@rgoldberg rgoldberg marked this pull request as ready for review January 6, 2022 08:10
@remkop remkop added this to the 4.6.3 milestone Jan 14, 2022
remkop
remkop previously approved these changes Jan 14, 2022
Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

I finally had time to look at this.
Looks good. Thank you for the contribution and your patience!

I added some minor feedback but I'm not too fussed, your proposed code is also fine.

Can you add some tests?
I would like to see a test that demonstrates the problem (fails with the current impl, and passes with the proposed change).

src/main/java/picocli/CommandLine.java Outdated Show resolved Hide resolved
src/main/java/picocli/CommandLine.java Show resolved Hide resolved
@rgoldberg
Copy link
Contributor Author

Thanks. Will look over feedback and modify the PR in the next few days.

Was planning to add tests (as per my initial description in this PR), just didn't want to make them until I had your feedback (e.g., if you wanted the existing behavior, my whole PR would never be accepted, so tests wouldn't ever be used, etc.)

@remkop
Copy link
Owner

remkop commented Jan 19, 2022

FYI, I will probably do the 4.6.3 release when this is merged. :-)

@rgoldberg
Copy link
Contributor Author

Sorry for delay. Will try to get to this soon. Thanks for heads up.

@remkop
Copy link
Owner

remkop commented Jan 29, 2022

Hi @rgoldberg, just wanted to check if this is still on your radar. :-)

@remkop
Copy link
Owner

remkop commented Feb 5, 2022

Hi @rgoldberg, if you don't think you will be able to work on this in the next week or so, that is not a problem: we can simply add it to the next release.
That said, I can wait a bit, I would like to include it, but I would like to do a picocli release in the next week or so.
Can you let me know your plans?

@rgoldberg
Copy link
Contributor Author

Will get it done this weekend. Sorry for delay.

@remkop
Copy link
Owner

remkop commented Feb 7, 2022

@rgoldberg I can see your feedback on my comments, that you commented "Done", but I don't see any additional commits yet. Did you push your changes?

@rgoldberg
Copy link
Contributor Author

No. I committed the changes locally. Writing the test now. Will push all soon.

@rgoldberg
Copy link
Contributor Author

Added tests. Fails on current master, but passes on this PR branch.

Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

Looks great, thank you so much!

@remkop remkop merged commit edd902c into remkop:master Feb 7, 2022
remkop added a commit that referenced this pull request Feb 7, 2022
@rgoldberg rgoldberg deleted the modify-a-command-spec-aliases branch February 8, 2022 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modifying a CommandSpec's aliases if it's already a subcommand
2 participants