Add section heads to help commands.#9953
Add section heads to help commands.#9953richsalz wants to merge 6 commits intoopenssl:masterfrom richsalz:app-help-categories
Conversation
|
If someone can take a look at the output, I'd like an opinion of there should be blank line before each section. |
|
First pass done; all programs with >=20 help lines: asn1parse ca cms crl dgst ecparam enc ocsp pkcs12 pkcs8 pkeyutl req rsa rsautl s_client s_server smime speed ts verify x509 |
|
All commands done, out of WIP, ready for review. |
|
in order to entice folks to look at and review this, here's some output: For common options (OPT_[VSXR]_OPTIONS in |
|
fixup commit pushed. |
levitte
left a comment
There was a problem hiding this comment.
I'm getting blurry-eyed... I'll get back on this, but wanted to at least get a few findings out.
|
As for the Input, Output, Input/Output: if there were a bunch of options, or I thought it made sense, I divided them into sections, otherwise I didn't. I agree it might be better to go for consistency, but I also dislike a section that has only one option (yes sometimes that is unavoidable). I hope that this topic can be addressed in a separate PR. (Probably by someone else :) |
|
My approval still holds. |
|
ping @openssl/omc for 2nd review |
|
Another sample, to entice reviewers: |
|
Travis failures seem relevant, there seems to be some option duplication in some commands. |
|
Yeah, the rebase broke the apps/enc.c. There are added duplicate options in, out, and pass. |
|
OK, re-approved now. |
|
Another sample: |
|
For the record my approval still holds. |
Remove "Valid options" label, since all commands have sections (and [almost] always the first one is "General options"). Have "list --options" ignore section headers Reformat ts's additional help
|
My approval still holds. It would be nice if @openssl/omc provided the second review soon to avoid further merge conflicts. |
vdukhovni
left a comment
There was a problem hiding this comment.
Looks well motivated overall, but I am requesting some changes.
| "Print old-style (MD5) subject hash value"}, | ||
| #endif | ||
|
|
||
| OPT_SECTION("Certificate"), |
There was a problem hiding this comment.
This mixes certificate creation options with certificate output options, probably should be separate.
There was a problem hiding this comment.
The only output-related option I could see is "-nameopt" which I moved to output. Did you8 have others in mind?
| {"force_pubkey", OPT_FORCE_PUBKEY, '<', "Force the key to put inside certificate"}, | ||
| {"subj", OPT_SUBJ, 's', "Set or override certificate subject (and issuer)"}, | ||
|
|
||
| OPT_SECTION("CA"), |
There was a problem hiding this comment.
These are certificate creation options, should perhaps include some of the same from above.
There was a problem hiding this comment.
Can you give me more details? Which options should move, and to which section?
Address some of Viktor's feedback.
|
I believe we should focus on real errors/mistakes and not try to achieve perfection in this PR. IMO even in the state as it is now it would be major improvement over the current situation. |
paulidale
left a comment
There was a problem hiding this comment.
Looks good.
It there any rsason for the OPT_MORE_STR lines are required? Thye'd be construable from the raw test I suspect.
|
I don't understand your question. |
|
(PS: Want to tweak the labels so the 24hour hold timer works?) |
|
I'm wondering why there are OPT_MORE_STR lines in the text. They seem to be manually putting line breaks in places -- this could be automated. It's quite poissible I missing something egregious. |
|
The number really didn't change. Auto-wrapping the text would be more trouble than it's worth for the roughly-a-dozen lines: |
Remove "Valid options" label, since all commands have sections (and [almost] always the first one is "General options"). Have "list --options" ignore section headers Reformat ts's additional help Add output section Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #9953)
|
Merged to master. Thanks for the effort and endurance on of this one. |
Fixes: #9936
This is a WIP. When done I'll remove the TODO file.