Skip to content

feat: exported PrintMarkdown function#118

Merged
barrettj12 merged 1 commit intojuju:v3from
barrettj12:format-cmd
Oct 3, 2024
Merged

feat: exported PrintMarkdown function#118
barrettj12 merged 1 commit intojuju:v3from
barrettj12:format-cmd

Conversation

@barrettj12
Copy link
Copy Markdown
Contributor

@barrettj12 barrettj12 commented Aug 30, 2024

Motivation

The juju/cmd library already includes a way to format a given command as Markdown. However, currently this is buried deep inside the implementation details of the internal documentation command. As a result, given just a cmd.Command, it is not easy to print a Markdown document for it.

To get around this, we have had to use nasty hacks, such as for the hook tools, where we created a "fake"/artificial supercommand for the hook tools and ran the embedded documentation command to generate the docs.
https://github.com/juju/juju/blob/0ae8f81a9e5ecc3a50c35ea059861a000f3c6657/scripts/md-gen/hook-tools/main.go#L43-L60

Changes

This PR refactors this code to expose a top-level PrintMarkdown function. This function takes three arguments:

  • the cmd.Command to print the Markdown for;
  • an io.Writer which all output is printed to;
  • a MarkdownOptions struct whose values affect the outputted Markdown. The intent is that more fields can be added to this struct in response to future needs, while maintaining backward compability. Currently the options include:
    • whether or not to print a title
    • a prefix for the command usage (in our case this would be "juju ", as the supercommand name is not stored in the command)
    • functions to provide link targets for command names. These generally depend on the environment and context in which we are generating the documentation.

As we continue to move towards generated documentation, this new function should make the generation much easier. For example, there are other cmd.Command implementations in juju (such as the agent introspection tools) which we want to create docs for. The intention furthermore is that the existing documentation command will be refactored to use this function internally.

I'm not guaranteeing the output will be exactly the same as before, but it should have parity information-wise and at least not regress in any way in terms of formatting.

As this PR extends the public API of this package in a backwards-compatible way, it may necessitate a new minor release v3.1.0.

Example usage

An example usage of this function could be:

file, err := os.Create("mydoc.md")
cmd.PrintMarkdown(file, &myCommand, cmd.MarkdownOptions{})
file.Close()

Documentation changes

Minor changes to generated Markdown:

  • All generated Markdown docs previously had a redundant horizontal line at the end. This has now been removed.
  • Command aliases are printed.

QA

Pull the 3.5 branch of juju/juju. Build the client and generate the documentation.

make go-client-build
juju documentation --split --out=./docs

Commit the generated documentation so we can compare it with the new docs.

Pull this branch of juju/cmd. In the root directory of the juju/juju/3.5 branch, create a go.work file:

go work init .
go work edit -replace=github.com/juju/cmd/v3=[path/to/this/branch]

Rebuild the Juju client:

make go-client-build

Now delete the ./docs folder and regenerate the documentation to see the changes.

TODO

  • error handling
  • unit tests
  • QA with Juju, check diff with current documentation

Links

Jira card: JUJU-6627

Copy link
Copy Markdown
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

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

Seems like an reasonable improvement.

markdown.go Outdated
// given io.Writer. The MarkdownOptions can be provided to customise the
// output.
func PrintMarkdown(w io.Writer, cmd InfoCommand, opts MarkdownOptions) error {
// TODO: error handling ?
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.

Yeah, we need some. But it might be easier to use a bytes.Buffer, ignore the errors on that, then io.Copy the result to w. On the io.Copy, that is the only error we care about.

@SimonRichardson
Copy link
Copy Markdown
Member

I have concerns about the cmd package getting larger, but I don't think my concerns should stop this landing. This is more of keep this in mind. The more we offer out to the agent binaries (jujuc, jujud, et al) the larger they become, which impacts the size and start-up time, but more importantly the security posture of those commands. One wrong move offering an additional feature that isn't required for running in production can cause us to introduce a security issue.

I would rather we took a step back and think, is there another way we can decorate only the commands we want to decorate, and is there a way to only decorate them in a certain mode (debug mode?).

@barrettj12 barrettj12 requested a review from hpidcock September 10, 2024 20:04
@barrettj12 barrettj12 marked this pull request as ready for review September 10, 2024 20:04

// EscapeMarkdown returns a copy of the input string, in which any special
// Markdown characters (e.g. < > |) are escaped.
func EscapeMarkdown(raw string) string {
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.

Looks like EscapeMarkdown needs a test.
Also it seems like it doesn't escape all markdown https://spec.commonmark.org/current/ so perhaps the name should be more specific to the task at hand.

Also perhaps look at use test fixtures in a testdata directory to compare output to (rather than having to escape again in Go strings)

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.

EscapeMarkdown does a partial escape of certain Markdown characters that can cause problems in outputted Markdown docs. I wanna be conservative here and only escape characters which we know to be an issue. There are many special Markdown characters that we specifically don't want to escape, for example hyphens -, we want hyphenated plain-text lists to display as formatted bulletpoints in Markdown.

Maybe the name's not 100% accurate but I can't think of anything better right now. Perhaps PartialEscapeMarkdown but that doesn't really feel like an improvement. The doc comment could be improved at least, we don't escape "any special Markdown characters", only a subset of them.

FYI this is not new code, it was moved here from documentation.go as I think it makes more sense here. This is tested in the documentation suite already.

@Aflynn50 Aflynn50 self-requested a review September 30, 2024 14:34
Copy link
Copy Markdown

@Aflynn50 Aflynn50 left a comment

Choose a reason for hiding this comment

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

LGTM, had meant to review this earlier, but missed it in my queue!

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.

5 participants