Add default lazygit config generation in Config.md from JSON schema

[karimkhaleel]

• PR Description
  This uses the JSON schema generated in Add UserConfig jsonschema generation script #3039 to generate and replace the default lazygit config in Config.md when running go generate ./...

Relevant issue: #3441

The generated config contains all the entries that have default values set in user_config.go

Several notes:

• windowSize is an enum of normal, half, full, but we set it to an empty string in user_config.go. I set it to normal because the comment we have there says that normal is the default value.
• pager has a validation of minLength=1, but we set it to equal an empty string in user_config.go. Not sure what to do about this field, we explicitly set it to equal an empty string in the user config but require a min length of 1.
• commitPrefix's pattern and replace both also fail validation because they are set to be empty strings by default but have a min length validation of 1.

So at the moment we have 3 validation issues in the generated config that I am not sure how to fix.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

This is fantastic, really great work.

A couple of thoughts:

• I would like it if the order of the keys were the same as defined in the UserConfig struct. (Yeah, it isn't in master's Config.md either, but I guess that's because of sloppyness.) It would be nice if we could group related configs together, or move the most important ones to the top, etc, and have this preserved in the docs. I tried commenting out the two sort calls in the generator, but that didn't do it; I suppose that's because of the var schema map[string]any which has random iteration order, so you may have to replace that with an orderedmap.OrderedMap[string, any]. I spent 10 minutes on getting this to work, but didn't succeed, so I'll leave this to you. 😄
• Indentation was 2 in the previous doc, now it's 4; I'd prefer 2 because it saves some horizontal space.
• There are two blank lines in the comments for git.log.order and git.log.showGraph; these look a bit confusing, because at first glance it's not clear what key the comments belong to. I couldn't see where these come from, if we could avoid this it would be great. But we could also simply delete the blank lines at the source of the comments, that would be good enough for me.

To answer your questions:

• defaulting windowSize to "normal" seems right, we were just sloppy to set it to an empty string, and for the code that uses the config it doesn't make a difference.
• for pager and commitPrefix, I'd suggest to simply delete the min length settings; I don't think they provide much value.

Really awesome, I'm very happy about this. Let me know if there's anything I can do to help finish this.

[stefanhaller]

Awesome, thanks for addressing all my wishes. The solution with the orderedmap is really nice.

I have a few more, but this time I pushed WIP or fixup commits for them:

• The old, manually crafted default section had most of the comments as line comments after the value. I pushed a commit (81034ed) that makes it so that one-line descriptions go after the value, multi-line descriptions before the key. It's a matter of taste which one looks better; I don't have a strong opinion, I only changed it so that we're closer to how it was before. @jesseduffield, any opinion? For comparison, here is Karim's original version, and here is the one with line comments.
• I came up with a workaround for the missing # on blank lines in multi-line descriptions (bc7e29f), which means we could drop the removal of the empty lines again (7e7d1a8)

[stefanhaller]

These changes and the ones below in the same file could go into a separate commit (before this one).

[karimkhaleel]

Moved it out of this commit. Changed the history of the commits though. I think if I force push now the links you posted will break. I can add comments below yours with the updated hashes though.

[stefanhaller]

These days github is pretty good at retaining commits that are mentioned in comments. I think it's safe to force-push, I'm relatively sure that the links will continue to work. I'll mention the two commits again here just to be sure: 4a437b3 (before), 9267b2e (after)

[karimkhaleel]

I pushed a commit (81034ed) that makes it so that one-line descriptions go after the value

Nice! I like the way it looks with the line comments.

I came up with a workaround for the missing # on blank lines in multi-line descriptions (bc7e29f), which means we could drop the removal of the empty lines again (7e7d1a8)

This is great! I dropped the change that deleted those lines out of the user config. I think having the ability to add blank lines is important.

I was also thinking about adding some more logic to drop the entries that are deprecated from the generated yaml. What do you think @stefanhaller?

[karimkhaleel]

I pushed a new branch to avoid overwriting this one's commit history for now. This is what Config.md would look like with the removed deprecated configs.

[stefanhaller]

I was also thinking about adding some more logic to drop the entries that are deprecated from the generated yaml. What do you think @stefanhaller?

I would actually prefer to leave the deprecated configs in as long as we support them. I think it's good to document them too. No strong opinion though, again I'll leave the final decision about this to @jesseduffield.

[jesseduffield]

The old, manually crafted default section had most of the comments as line comments after the value. I pushed a commit (81034ed) that makes it so that one-line descriptions go after the value, multi-line descriptions before the key. It's a matter of taste which one looks better; I don't have a strong opinion, I only changed it so that we're closer to how it was before. @jesseduffield, any opinion? For comparison, here is Karim's original version, and here is the one with line comments.

Reading through it, I'd actually like to have all comments sitting above the line they refer to. That makes it more consistent and I think would make it look neater too.

I would actually prefer to leave the deprecated configs in as long as we support them. I think it's good to document them too. No strong opinion though, again I'll leave the final decision about this to @jesseduffield.

I agree: let's document deprecations. It prevents a user from having to read the code to understand how their current setup is working. If a user goes and makes use of a deprecated config and then we remove it, that's on them.

[jesseduffield]

Really great work. Left a couple comments

[jesseduffield]

I think we should add a hidden yaml comment which marks the beginning (and end if necessary) of the config so we can just replace that. Using ## Default isn't very specific. Also this allows us to tell devs not to update it manually. So something like:

<!-- START CONFIG YAML: AUTOMATICALLY GENERATED with `go generate ./..., DO NOT UPDATE MANUALLY -->
...
<!-- END CONFIG YAML -->

[jesseduffield]

I would just always show the comment on top of the line

[jesseduffield]

It would be nice if we could also separate keys that have comments with newlines, but I'm not sure how to do that after hacking it at for a couple minutes

[stefanhaller]

It doesn't seem to be possible: go-yaml/yaml#627

But also, since almost all keys (except for keybindings) have comments now, wouldn't that consume too much vertical space?

[jesseduffield]

I'm not concerned about vertical space, but seems it's a moot point if the library doesn't support it

[stefanhaller]

But we can insert the blank lines ourselves after marshalling; it's not that hard actually, and it does indeed make things much more readable:

diff --git a/pkg/jsonschema/generate_config_docs.go b/pkg/jsonschema/generate_config_docs.go
index 000aad53b..965477f2c 100644
--- a/pkg/jsonschema/generate_config_docs.go
+++ b/pkg/jsonschema/generate_config_docs.go
@@ -144,6 +144,30 @@ func parseNode(parent *Node, name string, value *orderedmap.OrderedMap) {
 	}
 }
 
+func insertBlankLines(buffer bytes.Buffer) bytes.Buffer {
+	lines := strings.Split(strings.TrimRight(buffer.String(), "\n"), "\n")
+
+	var newBuffer bytes.Buffer
+
+	previousIndent := -1
+	wasComment := false
+
+	for _, line := range lines {
+		trimmedLine := strings.TrimLeft(line, " ")
+		indent := len(line) - len(trimmedLine)
+		isComment := strings.HasPrefix(trimmedLine, "#")
+		if isComment && !wasComment && indent <= previousIndent {
+			newBuffer.WriteString("\n")
+		}
+		newBuffer.WriteString(line)
+		newBuffer.WriteString("\n")
+		previousIndent = indent
+		wasComment = isComment
+	}
+
+	return newBuffer
+}
+
 func writeToConfigDocs(buffer *bytes.Buffer) {
 	// Remove all `---` lines
 	strData := buffer.String()
@@ -235,5 +259,7 @@ func GenerateConfigDocs() {
 		}
 	}
 
+	buffer = insertBlankLines(buffer)
+
 	writeToConfigDocs(&buffer)
 }

[jesseduffield]

Ah nice, I'm happy with that approach

[stefanhaller]

@karimkhaleel Another thought: the original Config.md had Keybindings last, and I think that's good. We should reorder the UserConfig struct to move Keybinding to the end. (I'd do that as a separate commit at the end of the branch.)

[karimkhaleel]

I think I addressed all the issues raised. Liking the spaces before entries with comments. I think all that's left is cleaning up the commit history, right? Any suggestions on what I should squash?

[jesseduffield]

LGTM, great work. I'll let @stefanhaller advise on squashing of commits. I'd be happy for this to be squashed into a single commit because it doesn't touch much existing code but happy to defer to @stefanhaller 's judgement

[stefanhaller]

I agree, great work. (I think I said this before. 😄)

I'd keep the first two commits and the last one separate, and squash everything else into the third one. (Hope that's clear enough.)

[karimkhaleel]

Awesome! Thank you for your feedback!

[stefanhaller]

The perfectionist in me found a few more nits to pick, see below. I added fixups for all of them.

I'm going to squash them right away and merge to save another roundtrip, hoping they are uncontroversial enough.

[stefanhaller]

We are cutting off the last character, assuming that it is a newline, but we don't really check that. I think it's better to trim the space before splitting into lines, like we are doing in insertBlankLines. Changed in 4225893.

[stefanhaller]

This is a potential memory allocation, and since we are using a bytes.Buffer which is supposed to have smart buffering, it's more efficient to write the two things separately. Changed in fa16009.

[karimkhaleel]

Good catch!

[stefanhaller]

err is always nil here, so it doesn't make sense to include it in the error message. Also, when no formatting is needed, it's better to use errors.New instead of fmt.Errorf (see #3523). Changed in dd9f158.