-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add default lazygit config generation in Config.md from JSON schema #3565
Add default lazygit config generation in Config.md from JSON schema #3565
Conversation
This is fantastic, really great work. A couple of thoughts:
To answer your questions:
Really awesome, I'm very happy about this. Let me know if there's anything I can do to help finish this. |
963fa77
to
4a437b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -252,7 +252,7 @@ type PagingConfig struct { | |||
// diff-so-fancy | |||
// delta --dark --paging=never | |||
// ydiff -p cat -s --wrap --width={{columnWidth}} | |||
Pager PagerType `yaml:"pager" jsonschema:"minLength=1"` | |||
Pager PagerType `yaml:"pager"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes and the ones below in the same file could go into a separate commit (before this one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I like the way it looks with the line comments.
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? |
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. |
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. |
844652f
to
b72b62f
Compare
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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work. Left a couple comments
return | ||
} | ||
|
||
defaultSectionIndex := bytes.Index(markdown, []byte("## Default")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 -->
|
||
const IndentLevel = 2 | ||
|
||
func setComment(yamlNode *yaml.Node, description string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just always show the comment on top of the line
Kind: yaml.ScalarNode, | ||
Value: child.Name, | ||
} | ||
if child.Description != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not concerned about vertical space, but seems it's a moot point if the library doesn't support it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice, I'm happy with that approach
@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.) |
4e019d9
to
bdf297a
Compare
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.) |
bdf297a
to
88e57fd
Compare
Awesome! Thank you for your feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// Remove trailing newline | ||
config := newBuffer.Bytes() | ||
config = config[:len(config)-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
for _, line := range lines { | ||
if strings.TrimSpace(line) != "---" { | ||
newBuffer.WriteString(line + "\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
|
||
startConfigSectionIndex := bytes.Index(markdown, []byte(DocumentationCommentStart)) | ||
if startConfigSectionIndex == -1 { | ||
return fmt.Errorf("Default config starting comment not found, %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the generated default config in Config.md match the original order.
dd9f158
to
9b152d7
Compare
It used to be a common thing to have to update Config.md in a PR (and we often forgot despite the template). As of #3565 this is no longer necessary, so remove this from the template. Updating docs in general is still a good thing to think about, so we leave this in.
It used to be a common thing to have to update Config.md in a PR (and we often forgot despite the template). As of #3565 this is no longer necessary, so remove this from the template. Updating docs in general is still a good thing to think about, so we leave this in.
It used to be a common thing to have to update `Config.md` in a PR (and we often forgot despite the template). As of #3565 this is no longer necessary, so remove this from the template. Updating docs in general is still a good thing to think about, so we leave this in.
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 ofnormal, half, full
, but we set it to an empty string inuser_config.go
. I set it tonormal
because the comment we have there says thatnormal
is the default value.pager
has a validation ofminLength=1
, but we set it to equal an empty string inuser_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
'spattern
andreplace
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.
go generate ./...
)docs/Config.md
) have been updated if necessary