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

docs: combine CLI reference pages into a single page #530

Merged
merged 7 commits into from
Dec 5, 2024

Conversation

dwilding
Copy link
Contributor

@dwilding dwilding commented Dec 2, 2024

The purpose of this PR is to combine the pages under https://canonical-pebble.readthedocs-hosted.com/en/latest/reference/cli-commands/cli-commands/ into a single page

@dwilding
Copy link
Contributor Author

dwilding commented Dec 2, 2024

@IronCore864, @benhoyt this is a draft PR so that you can follow along with progress of creating a single CLI reference page. WIP page here: https://canonical-pebble--530.com.readthedocs.build/en/530/reference/cli-commands-wip/

The WIP page wasn't generated from the script. I'll update the script when we're happy with the structure of the page

A couple of things to note on the WIP page:

  • I dropped the "Usage" heading for each command. I'd consider these headings unnecessary and distracting, especially when everything is on a single page
  • I refactored the run command to prevent nested headings within Examples:
    • Created a section called "Environment variables" for the info about PEBBLE and PEBBLE_COPY_ONCE
    • The Examples section now focuses on how to run the daemon

My next actions:

  • Add an in-page TOC at the top of the page, grouping the commands like pebble -h does
  • Remove the current CLI reference pages, move the WIP page to the proper location, then update internal links
  • Figure out how/whether we can set up URL redirects to take care of external links
  • Update the generator and CI scripts to account for the updated format & location of the CLI reference

@dwilding
Copy link
Contributor Author

dwilding commented Dec 3, 2024

Done:

  • Add an in-page TOC at the top of the page, grouping the commands like pebble -h does
  • Remove the current CLI reference pages, move the WIP page to the proper location, then update internal links

New CLI reference page: https://canonical-pebble--530.com.readthedocs.build/en/530/reference/cli-commands/

The docs check is failing because I haven't updated the CI script yet

Next actions:

  • Figure out how/whether we can set up URL redirects to take care of external links
  • Update the generator and CI scripts to account for the updated format & location of the CLI reference

@IronCore864
Copy link
Contributor

The current preview looks good in general.

3 small things bother me:

  1. At the top, the list of all commands is logically organized (same as pebble help output I suppose), so the commands that follow are in that order as well, for example, run should be the first. But the commands are alphabetically ordered, which means if I click the first command at the top, it jumps below. I think it would be nicer if the commands were ordered the same as the list at the top.
  2. The table of contents on the right doesn't really help, and it hinders searches, if I want to search for a specific command on the page by Ctrl+F, it sometimes lands there, which isn't super helpful because you need to click on it again. Maybe we could remove the ToC for this page only?
  3. If we are to keep the ToC: Since some commands don't have examples, the ToC isn't pretty and consistent IMHO. Maybe we could exclude level 3 headings for this page?

@benhoyt
Copy link
Contributor

benhoyt commented Dec 3, 2024

At the top, the list of all commands is logically organized (same as pebble help output I suppose), so the commands that follow are in that order as well, for example, run should be the first. But the commands are alphabetically ordered, which means if I click the first command at the top, it jumps below. I think it would be nicer if the commands were ordered the same as the list at the top.

I wasn't sure about that. But I do quite like the logical grouping. However, I think the alpha grouping is useful in the page itself and in the right-hand ToC. So I suggest keeping as is but maybe making explicit what's going on. For example, at the top, say "The pebble command has the following subcommands, ordered by logical group:" and then below that, "Below are the commands ordered alphabetically:"

The table of contents on the right doesn't really help, and it hinders searches, if I want to search for a specific command on the page by Ctrl+F, it sometimes lands there, which isn't super helpful because you need to click on it again. Maybe we could remove the ToC for this page only?

I disagree here. I think the right-hand ToC is quite nice. You can either Ctrl-F or use the ToC. And it's consistent with other long Canonical docs pages.

If we are to keep the ToC: Since some commands don't have examples, the ToC isn't pretty and consistent IMHO. Maybe we could exclude level 3 headings for this page?

I agree with this -- let's drop the "Examples" and "How it works" level.

@dwilding
Copy link
Contributor Author

dwilding commented Dec 3, 2024

+1 for dropping the "Examples" and "How it works" level from the right-hand TOC. I'll work on that now

For the command listing at the top - I'll make it clearer what's going on. I also propose dropping the code font from these links. It's not ambiguous that these are the command names, and the uniform sea of code font makes it harder to parse quickly

@IronCore864
Copy link
Contributor

FYI, I think this script might need some refactoring because we removed those separate pages, see this error here.

@dwilding
Copy link
Contributor Author

dwilding commented Dec 3, 2024

FYI, I think this script might need some refactoring because we removed those separate pages, see this error here.

Yep I'll update that as part of this PR too - thanks!

@dwilding
Copy link
Contributor Author

dwilding commented Dec 4, 2024

I've updated the generator script to account for the new format & location of the CLI reference.

Previously the script created .md files for commands that didn't have .md files. Now that the commands are in a single .md file, I've removed this functionality (which let me simplify the script quite a bit). If we release a new command in the future, we'll need to manually add a corresponding section in /docs/reference/cli-commands.md.

Next actions:

  • Resolve conflicts coming from 9cb0ecf and update any new xrefs
  • Figure out how/whether we can set up URL redirects to take care of external links

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

I realise this is only a draft, but I had a quick look. Nice work! The CLI command reference has only been around for a few months, so I'm not worried about breaking those links -- probably not worth the time trade-off.

logger.info("Processing doc for command %s.", cmd)
markers = Markers(cmd)
if markers.start not in text:
logger.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this an exception now? I'm concerned that if we add a new Pebble command and run this, it'll just log something, whereas that should probably be a hard failure so we don't forget to add it.

Or, better, the script should add it to the right place (though that might be a bit more work).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Having the script adding a missing command is going to be complex, because it would also need adding to the logical grouping of commands if we wanted to fully automate. I don't think it's worth the effort really - and this is surely going to be a rare situation

I'd rather break the build if there's a command missing from the doc. I'll change the script so that we raise an exception

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that sounds fine to me for now -- thanks.

@dwilding
Copy link
Contributor Author

dwilding commented Dec 4, 2024

I realise this is only a draft, but I had a quick look. Nice work! The CLI command reference has only been around for a few months, so I'm not worried about breaking those links -- probably not worth the time trade-off.

That's fair - I'll spend a few minutes to understand the redirect mechanism, then stop if it's going to be a lot of work to set up

@dwilding
Copy link
Contributor Author

dwilding commented Dec 5, 2024

All changes are done. I can't find a convenient way to add command-level redirects in the code, so we'll leave this out

Thank you very much @IronCore864 & @benhoyt for your advice and reviews!

@dwilding dwilding marked this pull request as ready for review December 5, 2024 01:35
@dwilding dwilding merged commit 09ed46c into canonical:master Dec 5, 2024
18 checks passed
@dwilding dwilding deleted the onepage-cli branch December 5, 2024 01:36
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