chore: merge v3 to master #124
Conversation
Links to a different section of text in markdown should use - instead of _ for links with spaces. Also adds a newline before the line break marker in the index.
The usage was only printed if the command has arguments, common for `list` style commands. But these commands should also have a usage section in the docs.
juju#122 # Description This PR recreates juju#120. This PR makes some minor QoL improvements: 1. (First commit) Links to a different section of text in markdown should use - instead of _ for links with spaces. If a command has subcommands, the generated markdown currently resemble `#command_subcommand` and wouldn't properly navigate, instead `#command-subcommand` works. 2. (First commit) There is no line break between the end of the index file and the markdown separator `----` which causes issues when converting the markdown to other formats. 3. (Second commit) Always print command usage. The usage is only printed if a command has arguments, commonly empty for `list` style commands. But these commands should also have a usage section in the docs.
Update doc headings from UpperCase(<sub-command>) to LowerCase(<base-command> <sub-command>). E.g. a command like `juju add-controller` would receive a title of `ADD-CONTROLLER` but is now `juju add-controller` to better reflect the CLI usage. This shouldn't affect Juju documentation because there the title is often excluded since each command is documented on a different page, but for other tools using this package the title is more useful.
juju#123 This PR updates the documentation markdown generator. Specifically, it changes document headings from UpperCase(<sub-command>) to LowerCase(<base-command> <sub-command>). E.g. a command like `juju add-controller` would include a title of `ADD-CONTROLLER` but with this change becomes `juju add-controller` and better reflects the CLI usage. This shouldn't affect Juju documentation because there the title is often excluded since each command is documented on a different page, but for other tools using this package the title is more useful. This change was prompted by the `jimmctl` reference docs at https://canonical-jaas-documentation.readthedocs-hosted.com/en/latest/reference/jimmctl/ cc @tmihoc Partially fixes [JUJU-7372](https://warthogs.atlassian.net/browse/JUJU-7372) [JUJU-7372]: https://warthogs.atlassian.net/browse/JUJU-7372?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
There was a problem hiding this comment.
We're starting to paint ourselves into a corner.
The documentation command (if we should have one at all) should have just outputted yaml or json, then a script should have consumed that and formatted correctly depending on the markdown required.
This could be considered a breaking change.
| @@ -357,7 +357,7 @@ func (c *documentationCommand) linkForCommand(cmd string) string { | |||
| func (c *documentationCommand) formatCommand(ref commandReference, title bool, commandSeq []string) string { | |||
| var fmtedTitle string | |||
| if title { | |||
| fmtedTitle = strings.ToUpper(strings.Join(commandSeq[1:], " ")) | |||
There was a problem hiding this comment.
This is just a mess. We're changing the documentation command because the output doesn't match the current implementation of the rendering framework we're using at the time.
If this isn't tail-wagging the dog, I don't know what it is.
There was a problem hiding this comment.
Can you elaborate on this? On the line you highlighted for example, I made the change as Teodora suggested that titles shouldn't be COMMAND but rather <binary> command, i.e. not the command alone in uppercase, but rather the tool name and then the command in lowercase.
This isn't related to the rendering framework.
There was a problem hiding this comment.
I made the change as Teodora suggested that titles shouldn't be
COMMANDbut rather<binary> command,
That's a breaking change. This package is publicly used and versioned, and as such this should be a new change. We have to hold our changes accountable here. I know I'm being pedantic, and there might not be other consumers of this package that are using this piece of code, but you don't know that.
Assume that you were using this documentation command, and that upstream did change the command output. It's unexpected and surprising, even if it's just "rendering". We should be careful of the consequences of just changing something without seeing it through to the end (a version bump).
I'm not going to stop this landing, but at some-point we should just call it done and come up with a better way. This isn't it.
There was a problem hiding this comment.
Ah I understand you now. Totally fair, I was working under the impression that, like the juju/juju repo, we weren't so strict with backwards compatibility here. But I see that is not the case after reading your response, so I'll keep that in mind going forward.
Forward merge v3 changes to master including changes from: