-
-
Notifications
You must be signed in to change notification settings - Fork 11k
Add section heads to help commands. #9953
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
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 |
t8m
left a comment
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 a few nits
|
fixup commit pushed. |
t8m
left a comment
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.
Good work!
levitte
left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mixes certificate creation options with certificate output options, probably should be separate.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are certificate creation options, should perhaps include some of the same from above.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <tmraz@fedoraproject.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (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.