Allow aliases of a CommandSpec that is already a subcommand to be properly & consistently modified. #1529
Conversation
…perly & consistently modified. Resolves #1528
There was a problem hiding this comment.
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).
|
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.) |
|
FYI, I will probably do the 4.6.3 release when this is merged. :-) |
|
Sorry for delay. Will try to get to this soon. Thanks for heads up. |
|
Hi @rgoldberg, just wanted to check if this is still on your radar. :-) |
|
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. |
|
Will get it done this weekend. Sorry for delay. |
|
@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? |
|
No. I committed the changes locally. Writing the test now. Will push all soon. |
|
Added tests. Fails on current master, but passes on this PR branch. |
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.