Skip to content

Conversation

tertsdiepraam
Copy link
Member

@tertsdiepraam tertsdiepraam commented Feb 22, 2022

Experimental implementation of #2816

I wanted to test my idea from #2816 and gather some feedback on it, so here we are :)

The macro reads the help.md file at the root of the crate extracts the section that is specified as an argument (matching the section is not dependent on whitespace and case). The implementation is a bit convoluted because I didn't want to pull in syn as a dependency, but I think it works.

Thoughts?

For some context, my semi-long term goal for documentation is:

  • Write documentation for all utils in help.md files in markdown.
  • Use this markdown verbatim in the user docs.
  • Generate manpages directly from the docs using mdbook-man.
  • Render this markdown into a terminal-compatible representation at compile-time for --help.
  • (Possibly) create a macro that generates a clap app with all the usage, after_help, about set from the help.md.

@sylvestre
Copy link
Contributor

it is going to include directly the content into the binary or read the file at runtime ?

@tertsdiepraam
Copy link
Member Author

It reads the file at compile-time!

@tertsdiepraam
Copy link
Member Author

This is what it looks like after using cargo expand (i.e. it's just like before):

const ABOUT: &str = "Convert numbers from/to human-readable strings";
const LONG_HELP : & str = "UNIT options:\n   none   no auto-scaling is done; suffixes will trigger an error\n\n   auto   accept optional single/two letter suffix:\n\n          1K = 1000, 1Ki = 1024, 1M = 1000000, 1Mi = 1048576,\n\n   si     accept optional single letter suffix:\n\n          1K = 1000, 1M = 1000000, ...\n\n   iec    accept optional single letter suffix:\n\n          1K = 1024, 1M = 1048576, ...\n\n   iec-i  accept optional two-letter suffix:\n\n          1Ki = 1024, 1Mi = 1048576, ...\n\nFIELDS supports cut(1) style field ranges:\n  N    N\'th field, counted from 1\n  N-   from N\'th field, to end of line\n  N-M  from N\'th to M\'th field (inclusive)\n  -M   from first to M\'th field (inclusive)\n\n- all fields\nMultiple fields/ranges can be separated with commas" ;
const USAGE: &str = "{} [OPTION]... [NUMBER]...";

@sylvestre
Copy link
Contributor

I like this idea, @rivy what do you think?

@rivy
Copy link
Member

rivy commented Feb 23, 2022

I also like the idea. 👍🏻

Some random thoughts...

  1. Could we make the help file just little a more filled out, with enough info to be read on it's own (still preserving the cool auto document generation features that you've implemented)?
# <UTIL>

## about

...

## help

...
  1. Maybe the file could be named <util>.md so that they could be gathered into a single directory without overwriting each other; for example, if a user wants just a couple of independent (non-busybox) utilities and installs them in a local bin directory with their respective *.md files next to them.

@tertsdiepraam
Copy link
Member Author

tertsdiepraam commented Feb 23, 2022

Could we make the help file just little a more filled out, with enough info to be read on it's own (still preserving the cool auto document generation features that you've implemented)?

I'm not quite sure what you mean with this. Currently, you can add more markdown sections and they will just be ignored (the macro reads from the specified header until the next header). What information would you like to add?

Maybe the file could be named .md so that they could be gathered into a single directory without overwriting each other; for example, if a user wants just a couple of independent (non-busybox) utilities and installs them in a local bin directory with their respective *.md files next to them.

I'll have to figure out how to open the right file, but I think that should be possible. I'll try to implement this.

Another option that I just thought of, it could also be read from the README.md of the util. Although I'm not sure whether that's intuitive and it doesn't solve the issue you're describing.

@sylvestre sylvestre requested a review from rivy March 1, 2022 22:31
@sylvestre
Copy link
Contributor

@rivy are you ok ? :)

@tertsdiepraam tertsdiepraam force-pushed the long_help_file branch 2 times, most recently from a5af146 to 26f3771 Compare April 6, 2022 22:24
@tertsdiepraam tertsdiepraam marked this pull request as ready for review April 6, 2022 22:26
@tertsdiepraam
Copy link
Member Author

tertsdiepraam commented Apr 6, 2022

I have added a help_usage macro to also get the usage from the help file. To keep it readable as markdown the usage must be surround by markdown code fences and we can use the name of the util in the file, which is replaced by the execution phrase at runtime (or rather the name is replace with "{}" at compile-time which is then used as the input for the format_usage call where the execution phrase is put in.

@tertsdiepraam
Copy link
Member Author

Oh also the files are now in the <util>.md format as requested.

@tertsdiepraam tertsdiepraam requested a review from sylvestre April 7, 2022 10:55
@sylvestre
Copy link
Contributor

I life the idea. just a few questions:

  • I haven't seen any tests, is that expected?
  • if a section is missing (or misspell), do you generate an error ?
  • I guess all binaries will have to be updated to leverage this?
  • maybe it should be documented?

@tertsdiepraam
Copy link
Member Author

  • I haven't seen any tests, is that expected?

It's fairly hard to test as a unit test, but I could add add some tests to check the specific output of numfmt, for example.

  • if a section is missing (or misspell), do you generate an error ?

Yes, but ungracefully for now. It will spew an ugly failed to unwrap error. I'll update the unwraps to expects to make that a bit better.

  • I guess all binaries will have to be updated to leverage this?

Ideally, yes, but there's no rush. I've written down some future ideas for documentation here: #3181

  • maybe it should be documented?

I've tried to add some comprehensive docstrings, but it would be nice to also put it in a more discoverable place maybe. Where would you like to see this documented?

I'll get to these things sometime next week :)

@sylvestre
Copy link
Contributor

Sorry for the latency. I am happy to accept this change.
However what do you think about adding a comment in the code next to the calling functions saying "this function will parse the .md page to generate the help page"? Or something like that

@tertsdiepraam
Copy link
Member Author

I still wanted to address the things you mentioned earlier, but I've been very busy. I'll convert this to a draft until I've fixed those things up.

@tertsdiepraam tertsdiepraam marked this pull request as draft April 19, 2022 11:05
@tertsdiepraam
Copy link
Member Author

OK, I wrote some unit tests for the parsing of the sections. Also, I made the filename an argument of the macro, so instead of

const ABOUT: &str = help_section!("about");

it is now

const ABOUT: &str = help_section!("about", "numfmt.md");

This makes it immediately obvious where the info is coming from. I've also added some more documentation.

@tertsdiepraam tertsdiepraam marked this pull request as ready for review April 24, 2022 16:55
@sylvestre
Copy link
Contributor

Excellent idea, thanks

@sylvestre sylvestre force-pushed the long_help_file branch 2 times, most recently from ec72ff1 to 9f35f29 Compare May 10, 2022 15:14
@sylvestre
Copy link
Contributor

@tertsdiepraam sorry, i dropped the ball on this. Do you still want to land this? thanks

@tertsdiepraam
Copy link
Member Author

@sylvestre I still think this is useful indeed. I'll also have some time to add this to more utils in the coming weeks (in other PRs)

@sylvestre
Copy link
Contributor

Sure
maybe open good first bugs too ?

@sylvestre
Copy link
Contributor

I rebased it against main to see if it is still green

@tertsdiepraam
Copy link
Member Author

@sylvestre I think this is finally ready to be merged!

Copy link
Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

Your call if you want this in 0.0.15 or not :)

@tertsdiepraam
Copy link
Member Author

Sure, let's sneak it in :)

@tertsdiepraam tertsdiepraam merged commit 26b7099 into uutils:main Aug 20, 2022
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