-
Notifications
You must be signed in to change notification settings - Fork 510
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
Add doc for modules for --list
#2199
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Two things:
- This should have a test which shows that doc comments are printed, and that they're aligned.
- Since
Subcommand::format_doc
is only used inside ofSubcommand::list_module
, I would put it inside that function.
Ok I am a bit unsure why it shows other commits than mine as well. Maybe it is because I have rebased my fork in the meanwhile? I think it should follow your requirements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I made some minor changes, mainly alphabetizing argument lists (when they get long enough, I just sort the argument by name instead of having some semantic order) and un-hiding the doc from the JSON serialization, and fixing the JSON tests.
The extra commits are expected, if you rebase onto a new version of master, you'll see those extra commits, but I ultimately always squash merge, so they get squashed away when they get merged to master.
PR for #2194.