Skip to content

Consistent formatting of flags with args #10022

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.

Sign up for GitHub

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

Jump to bottom
Closed
richsalz wants to merge 9 commits into from
Closed

Consistent formatting of flags with args #10022

richsalz wants to merge 9 commits into from

Conversation

@richsalz
Copy link
Contributor

@richsalz richsalz commented Sep 25, 2019 •
edited by levitte
Loading

For documentation of all commands with "-flag arg" format them
consistently: "B<-flag> I<arg>"
Update find-doc-nits to complain if badly formatted strings are found.

levitte
levitte requested changes Sep 25, 2019
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These nits are repeated in quite a few places. Can you find them on your own or do you need someone to point each one out?

doc/man1/openssl-asn1parse.pod Outdated
Copy link
Member

@levitte levitte Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, should be [B<-inform> B<DER>|B<PEM>]

Why bold? Because they are actual keywords

Copy link
Contributor Author

@richsalz richsalz Sep 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, they are now I<DER>|I<PEM> throughout.

@richsalz
Copy link
Contributor Author

richsalz commented Sep 27, 2019

Fixes #9987

This was referenced Sep 27, 2019
Closed
Closed
@richsalz
Copy link
Contributor Author

richsalz commented Sep 29, 2019

I dislike the B<fooI<TYPE>bar> construct being used in some other pages, but I made a change to accomodate it. I also rewrite a sentence to avoid using B<foo<I<*>>

@richsalz
Copy link
Contributor Author

richsalz commented Sep 29, 2019

All other issues fixed.

levitte
levitte requested changes Sep 29, 2019
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's more of the same kind, but the mobile I do this from is so slow...

levitte
levitte reviewed Sep 30, 2019
View reviewed changes
levitte
levitte requested changes Sep 30, 2019
View reviewed changes
@levitte
Copy link
Member

levitte commented Sep 30, 2019 •
edited
Loading

As for the BIO_ADDR.pod changes, where you remove the emphasis. Typographically speaking, I'm actually fine with it, the emphasis isn't really needed. However, you say this in another PR:

The changes in BIO_ADDR.pod might be controversial, please take a look. The other option is to remove the embedded L<> tag, or allow them, or something else.

This indicates that decisions are made, not so much because of style, and more on the grounds of technical limitations in util/find-doc-nits. That's not a good reason to limit expressive freedom, which should be allowed as long as it doesn't blatantly break with conventions we want to follow.

(in other words, fix the script first)

@richsalz
Copy link
Contributor Author

richsalz commented Sep 30, 2019

fix the script first.

I'm kinda done with this PR.

@richsalz
Copy link
Contributor Author

richsalz commented Sep 30, 2019

Feedback addressed, or I disagreed, or I'm just plain done. :)

@richsalz
Copy link
Contributor Author

richsalz commented Sep 30, 2019

This PR has 1400 changes over 54 files. There are currently seven commits, but I am sure that I have squashed/rebased multiple times.

I think this is a huge improvement over what we currently have. It's not perfect, more could be done, but isn't that always the case?

@richsalz
Copy link
Contributor Author

richsalz commented Sep 30, 2019

I also removed the "Fixes" text from the commits since the scope of this has changed and it no longer fixes everything that people (well at least me and @levitte:) think should be done to address that issue.

levitte
levitte approved these changes Sep 30, 2019
View reviewed changes
Copy link
Member

@levitte levitte left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not happy approving this as it is, but that's what it takes to get the generally good bits in (because they are, and that's the grand majority of this PR), then so be it. Expect follow-up PRs

@levitte levitte added branch: master Applies to master branch approval: done This pull request has the required number of approvals approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels Sep 30, 2019
t8m
t8m requested changes Sep 30, 2019
View reviewed changes
@richsalz
Copy link
Contributor Author

richsalz commented Sep 30, 2019

Commit pushed to address @t8m's feedback.

Again, please squash this when merging.

@richsalz
Consistent formatting of flags with args
cc5ea57 
For documentation of all commands with "-flag arg" format them
consistently: "B<-flag> I<arg>"
Update find-doc-nits to complain if badly formatted strings are found.
@richsalz
Fix L reference to openssl pass phrase args
296f0e0
@richsalz
Fix formatting for I<DER|PEM> I<nm:v>
f88f35e 
Rewrite them as  I<DER>|I<PEM> I<nm>:I<v>
Fix nested formatting inside B<> and I<>
Update find-doc-utils to find instances of it.
@richsalz
Don't check nested B<> tags on generic pages
4dc1df5
@richsalz
Rewrite sentence to avoid a nested I<*> construct
4f992ac 
Also fix typo's.
@richsalz
Address some feedback.
f0a164f
@richsalz
Feedback addressed
92a187e
@richsalz
Feedback
f726e26
@richsalz richsalz mentioned this pull request Sep 30, 2019
Closed
t8m
t8m approved these changes Oct 1, 2019
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is in pretty good shape now. Thank you Rich for this tedious work.
@levitte will you reapprove and commit?

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 1, 2019
@levitte
Copy link
Member

levitte commented Oct 1, 2019

Sure

levitte pushed a commit that referenced this pull request Oct 1, 2019
@richsalz @levitte
Consistent formatting of flags with args
e876971 
For documentation of all commands with "-flag arg" format them
consistently: "B<-flag> I<arg>", except when arg is literal
(for example "B<-inform> B<PEM>|B<DER>")
Update find-doc-nits to complain if badly formatted strings are found.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #10022)
@levitte
Copy link
Member

levitte commented Oct 1, 2019

Merged.

e876971 Consistent formatting of flags with args

@levitte levitte closed this Oct 1, 2019
@richsalz richsalz deleted the flag-arg-nits branch October 1, 2019 13:03
@richsalz
Copy link
Contributor Author

richsalz commented Oct 1, 2019

Thanks everyone, particularly @levitte, for all the detail work on reviews.

This was referenced Oct 3, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte approved these changes

@t8m t8m t8m approved these changes

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@richsalz @levitte @t8m