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.

Sign up for GitHub

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

chore: merge v3 to master #124

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+89 −12
Open

chore: merge v3 to master #124

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c067220
chore: fix subcommand URI fragments
kian99 Dec 3, 2024
eb95f3f
fix: always print usage
kian99 Dec 4, 2024
0458506
Merge pull request #122 from kian99/minor-qol-tweaks-v3
jujubot Dec 4, 2024
5cebac4
chore: update doc headings
kian99 Jan 28, 2025
0359742
Merge pull request #123 from kian99/update-doc-heading
jujubot Jan 29, 2025
cbb3902
Merge branch 'v3' into merge-v3-to-master
kian99 Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion documentation.go
View file
Original file line number Diff line number Diff line 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:], " "))
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I made the change as Teodora suggested that titles shouldn't be COMMAND but 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

fmtedTitle = strings.ToLower(strings.Join(commandSeq, " "))
}

var buf bytes.Buffer