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

fix: Ensure aliases are visible in auto-generated markdown (#1545) #1649

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

devcollinss
Copy link

What

This change updates the markdown generation process to include aliases for both commands and arguments in the generated documentation.

Why

This change updates the markdown generation process to include aliases for both commands and arguments in the generated documentation.

Known limitations

N\A

FULL_HELP_DOCS.md Outdated Show resolved Hide resolved
@devcollinss
Copy link
Author

@willemneal, kindly review the PR. Also, could you please share your preferred means of communication—Discord or Telegram?

@willemneal
Copy link
Member

Darn! I wrote a longer response but it didn't get commented!

The summary is that you need to use get_visible_aliases instead. Using short aliases was a mistake we made early on, but for better readability in scripts it's better to use full words as it can be confusing reading it later which arg the short corresponds to. e.g. if two args start with s, what is -s? . So the short aliases can be for power users and they can find it with --help. But for the markdown I suggest we just highlight the normal aliases as these save space but are readable.

Also the aliases should appear inline with their argument.
e.g.

* --source-account, --source

Having them at the bottom will not help devs to discover them and this is more inline with the --help.

Note that rust field names become kebab case when converted to be a CLI arg. So you should be able to take your current setup and insert aliases by finding the corresponding arguments after converting the name kebab-case and adding the alias after like the example above.

Lastly, this bug seems like an issue for upstream clap-markdown itself since this would be the case for any markdown generated by it not just ours. However, given the scope of this work I think this is a fine compromise and won't rely on waiting for upstream releases.

As for communication Discord is preferred, but perhaps best to open an issue on the discord Dev channel so that others can also reply. And feel free to tag me (sir - willem), just the letters, didn't want the full thing in plain text (although an LLM could still figure it out lol).

@devcollinss
Copy link
Author

@willemneal i just updated the repo. please review it

alias_list.push(arg_name.clone());
alias_list.extend(visible_aliases);

content.push_str(&format!(
Copy link
Member

Choose a reason for hiding this comment

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

This is still just adding the aliases to the end of the document. And it seems that you include the arg_name in the alias list resulting in:

**Argument --shell**: --shell

Rather you should first check if the arg has an alias and then if so find the locations of arg_name in content and insert the alias after it. --source-account, --source. I would also suggest splitting content into a vec of lines and passing that in. Then in the caller join the string again. Inserting into a large string can be very expensive. When inserting in the middle of a string, a new string is created and then first part is copied over, the new string is appended and then the end of the of the original string. So by splitting the string once you pay once for the copying and then editing each line is much cheaper. Then pay once more for the joining.

An interesting article about how Zed handles string insertions for editing files: https://zed.dev/blog/zed-decoded-rope-sumtree

Since our use case is non-interactive and a one time build it's not so important to be as efficient as possible, but I still think it's worth splitting into lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog (Not Ready)
Development

Successfully merging this pull request may close these issues.

2 participants