-
Notifications
You must be signed in to change notification settings - Fork 75
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
docs: include generated documentation for the CLI commands #3286
Conversation
1175008
to
a595703
Compare
Readthedocs is complaining about my indentation... @s-makin would you know why? How to fix that, if needed? Besides that, there are two words (customizable, artifacts) which are in American, and if I translate those to English it becomes inconsistent with all messages in the client itself hahahaha :( so I added them to the wordlist. Do we prefer this small inconsistency in the docs, or in the client itself? OR should we rewrite the whole client in British? |
(I could remove the attach config example completely from the command description, and reformat the security status list of items to use |
Re: the spelling inconsistency, I think since it's coming from the source code it's better for us to just add those spelling exceptions than to start refactoring the codebase into GB English 😬 |
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.
Some comments/suggestions - hopefully I've caught all the places that could be causing build issues on RTD, but if not, double check that all your bullet/numbered/unnumbered lists have an empty line before and after, and that anything you want rendered as a code block has a .. code-block::
directive. That should fix the errors you saw :) I can do a check of the rendering after the RTD builds properly.
a595703
to
c156447
Compare
Breaks: autogenerated CLI documentation is unformatted |
c156447
to
a81fe67
Compare
@s-makin addressed all comments according to what we spoke about. All good with the RTD build now. Please take another look when possible. |
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.
Few more little nits to make the rendering consistent.
Also, there's a broken link to the IRC channel somewhere. I tested the link and it does appear to just hang forever and doesn't even time out. Not sure why?
I would like to eventually see a table of cli flags with descriptions for each command (example), but that can be a future iteration. This is already a great improvement thank you! |
@orndorffgrant I like the suggestion and I could easily add it now. Not doing because it would look horrible :D |
Breaks: autogenerated CLI reference could include flags and options in the output |
a81fe67
to
d78efc9
Compare
@s-makin addressed all your points, please take another look |
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.
Just one final nitpick to make the bullet lists consistently formatted.
Ideally, I would also like to see some brief instructions on how the autogeneration works in the devdocs (where the autogeneration happens, what files to change, etc). Since we know we'll have follow-up work on this, and we know it won't be imminently, it would be good to have that documented for Future Us. I'd be ok with us including that in a separate PR, though, if we don't want to hold this one up.
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.
lgtm except some remaining minor nits from @s-makin
Signed-off-by: Renan Rodrigo <[email protected]>
d78efc9
to
ab99141
Compare
@s-makin last review addressed, |
Sounds good to me :) let's do that |
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.
Thanks for all the work on this!
Hmm the breaks thing didn't trigger because merging to |
Why is this needed?
This PR solves all of our problems because now we have a CLI reference, and it is generated from the actual source code.
Test Steps
check the docs