Skip to content

Automatic manapge generation with vg help --man - #5018

Open
faithokamoto wants to merge 9 commits into
masterfrom
subcmd-manpage
Open

Automatic manapge generation with vg help --man#5018
faithokamoto wants to merge 9 commits into
masterfrom
subcmd-manpage

Conversation

@faithokamoto

Copy link
Copy Markdown
Contributor

Changelog Entry

To be copied to the draft changelog by merger:

  • vg help --man will output a fully formed Markdown manpage
  • vg help subcommand will output the same thing as running vg subcommand, i.e. the default helptext

Description

BEHOLD MY WORKS. Ahem. Extension of #4979 but now the Better (TM) manpage is generated by vg help --man instead of a separate script. I've diffed it against the current manpage and what changed were:

  • A few subcommand blurbs were edited to be better (e.g. vg giraffe's dropped the "short" from before "read alignment")
  • Things are correctly alphabetized
  • Deprecated subcommands still have their helptext printed, they're just not in the lists at the top.

Because vg help now needs to be able to print helptexts for other commands, I have Subcommands save a helptext function. Some subcommands' helptext functions require extra arguments. I worked around this by defining extra standard-argument functions, e.g. see help_augment_default().

The main thing I'm worried about here is vg giraffe. I did some terrible no good very bad hacks to make its default helptext function work because the previous one really wanted to be passed a list of presets. If you have a better way to do that I'm all ears.

@faithokamoto

Copy link
Copy Markdown
Contributor Author

Un-hacked vg giraffe so now the helptext prints a fixed list of presets instead of dynamic generation. To make the list short enough I got hid sr-chaining, but we don't want people to use it anyhow. I also removed one of the default help function uses. vg augment used a completely separate system for a lot of its options and I don't see why it needed it. My best guess is that it was supposed to auto-generate stuff to force options to be registered correctly, but the system never really got used elsewhere and now the scripts/lint.py will enforce the rules so I just dumped src/option.hpp/.cpp

@adamnovak adamnovak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is a good idea.

But I don't think the square peg/round hole adapter for the Giraffe help function is going to work, because we actually need the option group object that gets built to produce help for half the options.

One approach to fix that could be not taking help functions at all and just running the main functions with "--help" as an argument. Another approach could be taking the help functions but shimming the difficult cases with a fake help function that calls into main. And the third approach is refactoring difficult cases like this so both the help function and the main function can get ahold of the objects they want to share (with globals or an enclosing class or a builder function or something).

I also think we could harmonize the manpage category/description system and the help category/description system, and that we could carve out a place to put a longer description instead of having two short ones.

Comment on lines +764 to +769
// A version taking only argv to make the subcommand registry happy
void help_giraffe_default(char** argv) {
GroupedOptionGroup empty_parser;
help_giraffe(argv, empty_parser, false);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this will really work; that parser object is where about half the options actually live, including most of the ones that change what the mapping algorithm does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But do we want to print the full help? Currently we don't have the manpage use the --help helptext; we only have the result of running vg giraffe without arguments, which is what I'm replicating here.

Comment thread src/option.hpp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I liked this option system, but apparently not enough to use it.

*/
Subcommand(std::string name, std::string description,
CommandCategory category,
std::vector<manpage_item> manpage_entries,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we expect one subcommand to have multiple entries? I couldn't find any, just some that have an entry and some that don't. If we only want 0 or 1 of something, there's now std::optional for that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Some do have multiple, e.g. see vg paths with a ridiculous four entries.

Comment on lines 59 to +99
/**
* Defines what kind of command each subcommand is.
*/
enum CommandCategory {
/// Some commands are part of the main build-graph, align, call variants pipeline
PIPELINE,
/// Some subcommands are important parts of the toolkit/swiss army knife for working with graphs and data
TOOLKIT,
/// Some commands are less important but potentially useful widgets that let you do a thing you might need
WIDGET,
/// Some commands are useful really only for developers
DEVELOPMENT,
/// Some commands we're trying to move away from
DEPRECATED
};

/**
* The sub-lists on the manpage to organize subcommands
*/
enum ManpageSection {
// Graph construction and indexing
SET_UP_GRAPH,
// Read mapping
MAP_READS,
// Downstream analyses
DOWNSTREAM,
// Working with read alignments
MANIPULATE_ALN,
// Graph and read statistics
GET_STATS,
// Manipulate a graph
MANIPULATE_GRAPH,
// Conversion between formats
CONVERT_FORMAT,
// Subgraph extraction
EXTRACT_GRAPH,
// Extremely specific analyses
RARE_NEEDS,
// Developer tools
DEV_TOOLS
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We're adding this second set of categories so we can more or less exactly reproduce the current manpage. But then we end up with both a command category and maybe a categorized manpage section on each command, except that deprecated-category commands don't need manpage sections. Some of these (DEVELOPMENT and DEV_TOOLS) sure sound like they mean the same thing.

I would try and combine these two systems into one. We could make it hierarchical and put each command in exactly one leaf category (like FORMAT_CONVERSION), which then can fall into a broader category (probably what is now WIDGET). Or we can give each command 0 to n tags. Or we can throw away the existing command categories and use only the categories we're currently using for the manpage, plus DEPRRECATED.

Also, we probably want these to be all verbs or all nouns; GET_STATS and RARE_NEEDS are different types of phrase.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is complicated due to the subcommands which show up in multiple sublists, such as vg paths. How do you think those should be handled?

Comment thread src/subcommand/subcommand.hpp
Comment thread src/subcommand/subcommand.hpp
static Subcommand vg_surject("surject", "map alignments onto specific paths", main_surject);
static Subcommand vg_surject("surject", "map alignments onto specific paths", WIDGET,
vector<manpage_item>{{CONVERT_FORMAT, "project graph alignments onto a linear reference (gam/gaf->bam/sam/cram)", ""}},
help_surject, main_surject);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have the existing subcommand short descriptions and the existing manpage short descriptions, but are they really different enough in the job that they do that we shouldn't just use one string for both?

I think what we really want here is a single short description for when the command appears in reference lists, and then a way to include a paragraph or two of explanation about the command. The wiki page links, when they exist, can sort of do that, but I don't think we aspire to have a separate 2-paragraph wiki page about every subcommand. When someone runs vg surject --help and still doesn't get it, they might check the vg surject manpage next, and it would be good if we could offer more detail.

I think ultimately we'd also want to have longer-than-fit-in-the-list explanations for individual options as well, but we don't have to do that yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For the ones that have multiple entries (e.g. vg paths) we use tailored descriptions for each use case. I mean the core problem there is that some commands do ridiculously many things, but short of splitting them up, we do want to be able to trumpet all functionalities

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.

2 participants