add option OrderCommandsByNameInHelpText #148
Conversation
|
Hey @lvermeulen wanted to let you know I saw your PR. I'm should be able to review this closer in late September after I return from vacation. Thanks, |
natemcmaster
left a comment
natemcmaster
left a comment
There was a problem hiding this comment.
Thanks for writing tests! Not everyone does that and it really helps that you did. I have no problem adding API. My feedback is mostly about where we put this new API.
| /// <summary> | ||
| /// Determines if commands are ordered by name in generated help text | ||
| /// </summary> | ||
| public bool OrderCommandsByNameInHelpText { get; set; } = true; |
There was a problem hiding this comment.
I think this property should be on DefaultHelpTextGenerator, not CommandLineApplication. Other Implementations of IHelptTextGenerator are not required to honor this, so I don’t think it belongs here. Plus, I’m trying to avoid making CommandLineApplication a “god class”.
There was a problem hiding this comment.
I hear you! Let me make those changes.
| /// <summary> | ||
| /// Determines if commands are ordered by name in generated help text | ||
| /// </summary> | ||
| public bool OrderCommandsByNameInHelpText { get; set; } = true; |
There was a problem hiding this comment.
One last request: can we name this SortCommandsByName? Once that's in, I'll be happy to merge this PR.
There was a problem hiding this comment.
Sure thing, I added a commit.
|
Thanks @lvermeulen! |
2 participants
Fixes #147