Skip to content
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

feat! rewrite to be for ROFF, not manual pages #15

Merged
merged 1 commit into from
Dec 23, 2021

Conversation

larswirzenius
Copy link

While looking at generating manual pages from clap:App (see, for
example, clap-rs/clap#3174), I reviewed the
roff crate. I found several things that I think could be improved:

  • It mixes concerns. Some of the crate functionality is aimed at
    generating source for manual pages, and not just generic documents
    using the ROFF language. This both makes the crate less useful (it's
    not suitable unless one uses the -man macro package for troff), and
    harder to use (it hard codes some assumptions about manual pages,
    such as what arguments .TH gets).
  • The manual section is an integer, which prevents a suffix such as in
    the openssl(1ssl) manual page.
  • There's no support for sub-sections.
  • There's no escaping of text lines with leading dots. ROFF
    interprets them as control lines. If one is creating ROFF from, say,
    Markdown, one needs to handle the leading dots oneself.
  • In general, the user of the crate needs to understand how to quote
    or escape things to avoid invoking ROFF accidentally.
  • There's little to no documentation of the API.

This commit introduces a complete rewrite to address my concerns. I'm
afraid I could not come up with a reasonable sequence of small changes
to get where I want the crate to be. The changes are radical, and
break backwards compatibility, so the version number will need to be
bumped.

A summary of the changes:

  • The crate now aims at generic ROFF support. There is no trace of
    manual page support anymore. For example,struct Roff knows nothing
    about manual page metadata, and there is no longer any support for
    sections.
  • Control and text lines are given to a Roff explicitly, and handled
    separately, to get various kinds of escaping and quoting correct.
  • If generating manual pages, a section is done by creating an "SH"
    control line, and then text lines for the content. Any higher level
    abstraction should, I think, be done above the level of the roff
    crate.
  • Text lines consist of inline elements, to represent text in fonts.
  • Text lines with leading periods are handled by inserting an
    invisible glyph.
  • The Troffable trait is gone, as it didn't seem useful anymore.
  • All public symbols have rudimentary documentation.
  • There's some unit testing. The pre-existing manual page rendering
    test is kept, with no change to the expected output, though the code
    to generate is adapted to the new API.

Copy link

@sondr3 sondr3 left a comment

Choose a reason for hiding this comment

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

At least to me this looks like an overall much better approach to the API and functionality of this crate, You also need to update the README to the new API and such.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

This is such an unequivocal improvement that most of my comments I would not consider blockers. Probably the only one is whether to expose a separate builder or not.

Let me know your thoughts on how much you would like to handle before merge.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
/// The line consist of the name of a built-in command or macro,
/// and some number of arguments. Arguments that contain spaces
/// will be enclosed on double quotation marks.
pub fn control<'a>(mut self, name: &'a str, args: impl Into<Vec<&'a str>>) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a separate builder or should we just make the Roff functions return -> &mut Self?

Copy link
Author

Choose a reason for hiding this comment

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

I found the separate builder more convenient in cases where the whole page can't be built at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like I'm missing something. Effectively the only difference is one would be a &mut self builder and the other is a self builder.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Dec 22, 2021

feat! rewrite to be for ROFF, not manual pages

FYI the lint commit failed because there is a missing : after the !. As the failure says, I can take care of it in the end of you don't feel like it.

While looking at generating manual pages from clap:App (see, for
example, <clap-rs/clap#3174>), I reviewed the
roff crate. I found several things that I think could be improved:

* It mixes concerns. Some of the crate functionality is aimed at
  generating source for manual pages, and not just generic documents
  using the ROFF language. This both makes the crate less useful (it's
  not suitable unless one uses the -man macro package for troff), and
  harder to use (it hard codes some assumptions about manual pages,
  such as what arguments .TH gets).
* The manual section is an integer, which prevents a suffix such as in
  the openssl(1ssl) manual page.
* There's no support for sub-sections.
* There's no escaping of text lines with leading dots. ROFF
  interprets them as control lines. If one is creating ROFF from, say,
  Markdown, one needs to handle the leading dots oneself.
* In general, the user of the crate needs to understand how to quote
  or escape things to avoid invoking ROFF accidentally.
* There's little to no documentation of the API.

This commit introduces a complete rewrite to address my concerns. I'm
afraid I could not come up with a reasonable sequence of small changes
to get where I want the crate to be. The changes are radical, and
break backwards compatibility, so the version number will need to be
bumped.

A summary of the changes:

* The crate now aims at generic ROFF support. There is no trace of
  manual page support anymore. For example,`struct Roff` knows nothing
  about manual page metadata, and there is no longer any support for
  sections.
* Control and text lines are given to a Roff explicitly, and handled
  separately, to get various kinds of escaping and quoting correct.
* If generating manual pages, a section is done by creating an "SH"
  control line, and then text lines for the content. Any higher level
  abstraction should, I think, be done above the level of the roff
  crate.
* Text lines consist of inline elements, to represent text in fonts.
* Text lines with leading periods are handled by inserting an
  invisible glyph.
* The Troffable trait is gone, as it didn't seem useful anymore.
* All public symbols have rudimentary documentation.
* There's some unit testing. The pre-existing manual page rendering
  test is kept, with no change to the expected output, though the code
  to generate is adapted to the new API.
@epage
Copy link
Contributor

epage commented Dec 23, 2021

Going to go ahead and merge.

  • Builder is unresolved
  • There are some cases where a fix was applied in one place but not others

Going to do a quick PR after this to fix some things up but just wanting to unblock progress on clap_man

@epage epage merged commit 4ddaccb into rust-cli:master Dec 23, 2021
@sondr3
Copy link

sondr3 commented Dec 23, 2021

I'll close my own PR here then, thanks for helping out @larswirzenius 😄

@epage
Copy link
Contributor

epage commented Dec 23, 2021

#16

@epage
Copy link
Contributor

epage commented Dec 23, 2021

roff 0.2 has been released.

@epage epage mentioned this pull request Dec 23, 2021
@larswirzenius larswirzenius deleted the liw/roff-only branch December 27, 2021 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants