Add UserConfig jsonschema generation script

[karimkhaleel]

• PR Description

jsonschema generation script with comments as descriptions and default values from the default UserConfig struct.

#2984

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.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

[karimkhaleel]

For some reason autoformatter did not want to cooperate 🤢

[stefanhaller]

This is a good start, but there are quite a few things missing.

First, and most important thing: we are doing this schema thing for a reason. We want to use it in VS Code (or maybe other editors that support yaml schemas) and get error checking and completion this way. It seems that you haven't tested this at all, because it totally doesn't work yet. 😄 So, the way to verify whether we're done is not to manually inspect the schema to see if it looks kind of right, but to actually use it in VS Code and see if it provides completions etc.

Reasons why it doesn't work yet:

• The keys in the schema are all uppercase, but ours start with a lowercase letter. To fix this, we need to change our yaml: struct tags to json: tags so that the jsonschema.Reflector uses them. This requires us to first switch our yaml serialization to use invopop/yaml so that we don't have to duplicate all the tags for both yaml: and json:.
  • This, however, requires us to do Remove yaml fork #2833 first, because switching to invopop/yaml would otherwise break null keybindings again.
• The generated schema uses "$schema": "https://json-schema.org/draft/2020-12/schema", but VS Code's validator apparently doesn't support this yet. I had to change this to "$schema": "http://json-schema.org/draft-07/schema#" to make it work.

One more less important thought: I'd make this a go:generate thing instead of a stand-alone script. I made #3035 as a preparation for that.

[stefanhaller]

I was wrong about the "$schema" property: this only causes a warning in VS Code when you open the schema file itself, but it doesn't prevent the schema from working in config.yml. So it looks like we needn't do anything about that.

[karimkhaleel]

Thank you for the comments!

You're right, I probably should have done some tests on the real project 😆 I was testing it with some dummy schemas.

I'm currently working on the naming issue. If we want to retain the default values from the GetDefaultConfig struct, I'll need to add a file to the vendor directory of the invopop/jsonschema package. I want to hook into the behavior of assigning field names and generate a mapping between the tag names and the struct names. Would this be better to do in a fork or just directly in the vendor directory? I would normally just monkeypatch this sort of thing in python.

About the json vs yaml tag:
If we set FieldNametag to yaml then it will pick up on the names we set there, while still being able to use the yaml serializer we have now.
#2984 (comment)

Going to work on some more of the jsonschema tags as well, using https://github.com/jesseduffield/lazygit/pull/2859/files as a reference.

Regarding the go:generate command, this is my first encounter with it. If understand correctly, we simply need to add a comment that triggers the schema generation script? Are we trying to configure the project so that executing the go:generate command from the project's root will recursively run all the go:generate commands defined in its subdirectories? If so, should I be adding a go:generate comment in any location other than the jsonschema/main.go file included in this PR?

[stefanhaller]

I'll need to add a file to the vendor directory of the invopop/jsonschema package.

Can you explain more why this is necessary? Changing things in the vendor directory is not an option, so you'd have to create a fork. But I think forking should always be the last resort, so I'd like to understand better what you're trying to do and why it requires forking.

Have you seen the KeyNamer field of Reflector; might this help?

About the json vs yaml tag: If we set FieldNametag to yaml then it will pick up on the names we set there, while still being able to use the yaml serializer we have now.

Hm, reading the discussion in invopop/jsonschema#28 I got the impression that using yaml tags is not the way to go, and changing them to json is. But if this works, fine with me.

Regarding the go:generate command, this is my first encounter with it. If understand correctly, we simply need to add a comment that triggers the schema generation script? Are we trying to configure the project so that executing the go:generate command from the project's root will recursively run all the go:generate commands defined in its subdirectories? If so, should I be adding a go:generate comment in any location other than the jsonschema/main.go file included in this PR?

The typical pattern for go:generate is to have two files: the first has the implementation (basically the code in your scripts/jsonschema/main.go), and contains a //go:generate comment that calls the second file; the second file is a small one that just has a main() function which calls the code in the first one, and it has a comment //go:build ignore so that its main function doesn't conflict with the real one. If you rebase onto master, you can use the cheatsheet generator as an example.

I'd also suggest to put the code in pkg/jsonschema, and the generated schema in schema/config.json.

[karimkhaleel]

These are the changes I am thinking of making in a fork: 070cfa4

What I am trying to do is use the struct key names from the GetDefaultConfig struct to access the properties that the invopop/jsonschema package is generating. I am trying to get the values from the default config and set them to the Default fields in the output of invopop/jsonschema.

I looked at the KeyNamer field of reflector and it unfortunately does not have access to the original name of the struct key that I am looking for. I might be missing something since I am still not entirely familiar with Go, but I am pretty sure it would be impossible to retrieve the value that I am looking for unless I modify the behavior of reflectFieldName. I guess I could implement something that will build a similar structure to what invopop/jsonschema is already doing and then use that as a reference, but that seems a little rough.

I also refactored the code like you recommended. https://github.com/karimkhaleel/lazygit/tree/jsonschema-userconfig-modified-vendor

Question about go generate: Would we have to invoke go generate from different directories in the CI pipeline? Or is there some way to run all of them at once?

[stefanhaller]

Hm, I see. Yeah, it would be possible to reflect the structs on our side and build a mapping table of field names to tag names, but I agree that it sucks to have to do that when invopop/jsonschema already has all the code for that. I don't see another option than forking either, then. It would be worth filing an issue at invopop and seeing if they would be up for accepting a PR for the change.

I also refactored the code like you recommended.

Nice, this is going in a good direction. Can we make the name of the schema file config.json instead of schema.json? We might add schemas for other files in the future (e.g. for state.yml, although that wouldn't be as useful since users are not supposed to edit that).

Question about go generate: Would we have to invoke go generate from different directories in the CI pipeline? Or is there some way to run all of them at once?

Generating the schema is something that a developer would do, this doesn't happen on CI. We already have a check on CI that the generated schema is up to date; this would automatically take care of this new one, too. See 5ccc95b.

Running them all at once (locally) is done with go generate ./..., or make generate which does the same.

[stefanhaller]

I noticed another issue: invopop/jsonschema marks all keys as required unless they are tagged with omitempty. We are tagging only a few keys with omitempty though, but in our config nothing is required, really. So it would be good to find a way to make omitempty the default, so that we don't have to add it everywhere.

[karimkhaleel]

I noticed another issue: invopop/jsonschema marks all keys as required unless they are tagged with omitempty.

fixed with a flag to invopop/jsonschema

Can we make the name of the schema file config.json instead of schema.json?

done

I don't see another option than forking either, then. It would be worth filing an issue at invopop and seeing if they would be up for accepting a PR for the change.

Forked and included it in this changeset. I'll raise an issue and submit a PR, should I reference this thread?

Generating the schema is something that a developer would do, this doesn't happen on CI. We already have a check on CI that the generated schema is up to date; this would automatically take care of this new one, too. See 5ccc95b.

ah, very clever :)

[karimkhaleel]

I also added some jsonschema tags. I spent way more time than I would like to admit doing it programatically, and using the jsonschema from here as a reference: https://github.com/jesseduffield/lazygit/pull/2859/files. The oneOf property is giving me some trouble though. I will continue to attempt to programatically get them out of the reference schema but I need some more time for that.

Also I can't seem to figure out how to appease the check auto generated files job. I ran the go generate ./... command and it's not showing me any diffs on my machine.

Just noticed this too :)

[stefanhaller]

This is coming along really nicely, great progress.

It will take me a few days to review this thoroughly; I noticed a few things already though:

• when trying to set a keybinding to null, you get "Incorrect type. Expected string". Emily solved this with a "type": ["string", "null"] on the keybinding type. (I told you we should do Remove yaml fork #2833 first, then we wouldn't have this problem. 😉)
• When I run go generate ./... locally, I get a modified schema, like on CI. The only way I can explain this is that you had the schema open in VS Code, and hit Command-S before committing it, and you have a different auto-formatter than I do. Not sure what else could explain this.
• I find that all the indirections through $defs make the schema unnecessarily complicated, and I wonder if it's easier to work with when setting DoNotReference to true. (Not that it matters for end users.)

[karimkhaleel]

I find that all the indirections through $defs make the schema unnecessarily complicated, and I wonder if it's easier to work with when setting DoNotReference to true. (Not that it matters for end users.)

Done.

When I run go generate ./... locally, I get a modified schema, like on CI. The only way I can explain this is that you had the schema open in VS Code, and hit Command-S before committing it, and you have a different auto-formatter than I do. Not sure what else could explain this.

I don't have any editors open when doing this. I swear! 😆 I repeated this several times already and can't seem to make it produce a diff on my desktop. I tried running go generate on my laptop and it did indeed produce a diff. Going to push from there I guess.

For some reason my desktop wants to remove the edit examples:

I think it has something to do with how the jsonschema package is running on my laptop vs desktop. Very strange.

[karimkhaleel]

Oh. It still fails the CI check. I don't know what I can do about this. Maybe you could try running the generate command and commiting the file your computer produces? Maybe there is some issue with both of my machines.

[karimkhaleel]

I find that all the indirections through $defs make the schema unnecessarily complicated, and I wonder if it's easier to work with when setting DoNotReference to true. (Not that it matters for end users.)

Done.

Undone. It breaks the way that I am getting the default values out of the default config.

when trying to set a keybinding to null, you get "Incorrect type. Expected string". Emily solved this with a "type": ["string", "null"] on the keybinding type. (I told you we should do #2833 first, then we wouldn't have this problem. 😉)

I'll take a look at this issue and see if I can make sense of it. Aren't we going to have to ask users to update their configs though?

[stefanhaller]

when trying to set a keybinding to null, you get "Incorrect type. Expected string". Emily solved this with a "type": ["string", "null"] on the keybinding type. (I told you we should do #2833 first, then we wouldn't have this problem. 😉)

I'll take a look at this issue and see if I can make sense of it. Aren't we going to have to ask users to update their configs though?

We can do this in phases. First we only add support for <disabled>, but keep using our yaml fork which supports null. That way we don't break people's config. We still need to tell them (via release notes or whatever) that null is deprecated. In the schema we'd only support string, so people who use the schema when editing their config already get alerted that null is not valid (although it still works).

Then, after enough time has passed, we remove our fork and stop supporting null that way.

[stefanhaller]

OK, the CI failure is a bit puzzling. When I generate the schema locally on your branch, I don't get any modifications compared to what's checked in. Still, the CI check fails, and this seems to be caused by the check not really running against the tip of your branch, but against a synthesized merge commit that pretends that your branch was merged to master already. I find this confusing in a way, but it also makes sense, because it alerts you that you would break master if you would merge now.

The solution of course is to simply rebase onto master, this should fix the check.

[karimkhaleel]

Still, the CI check fails, and this seems to be caused by the check not really running against the tip of your branch, but against a synthesized merge commit that pretends that your branch was merged to master already.

I guess that makes sense.

We can do this in phases. First we only add support for , but keep using our yaml fork which supports null.

I'll look into this then.

I'll also try to get the default values insertion code to work with the DoNotReference flag. And I'll also see if I can get the oneOf tags set up.

[karimkhaleel]

Fixed it up so that DoNotReference works with the default values extraction. Had some issues with the AuthorColors, BranchColors, and CommitPrefixes values. They got set to null for some reason.

[stefanhaller]

I'm sorry for the delay, I kind of lost track of who's waiting for whom here. 😅 Do I understand right that you consider this ready from your side?

I pushed one small commit to fix two comments. Nothing to do with your changes, this was already wrong on master, but we might include it here while we're at it.

I think it would make sense to squash all commits into one, could you please do that and rebase on master?

I have a few more minor things below, but then I think we're good to go. This is looking really good now.

[stefanhaller]

These examples don't seem to work; the only one that shows up in the schema is "diff-so-fancy". I'm not sure this is because of the whitespace or because of the = in the values. I also couldn't figure out how to fix it; adding single quotes around the values doesn't help.

[karimkhaleel]

could be fixed using this approach: cf48f4d

otherwise we can just remove this

[stefanhaller]

Nice, I like this.

[karimkhaleel]

Do you think we should just keep these jsonschema relevant functions in the user_config file or should we put them in a different file in the same package?

I guess we could also define the type and the extend function in the jsonschema package and import it into user_config

[stefanhaller]

Since we define most of the rest of what goes into the schema in user_config.go, I think these extra things might as well be there, too. Not a strong opinion, I could go either way, and since @jesseduffield usually has a better sense of what should go where, I'll let him decide.

[stefanhaller]

These examples don't look right. This is the container for all keybindings, not a single key binding, so it doesn't make sense to have this here. It also didn't make it into the schema, probably because you can't have examples on an object.

You'd really want to have these on every single keybinding, but I'm not sure how to achieve that, or how hard we should try.

[karimkhaleel]

Removed in: dfddb86

could use JSONSchemaProperty(prop string) any from the jsonschema library on all of the keybinding configs to add these examples to each entry but that's kind of 🤢 in my opinion.

[stefanhaller]

Couldn't you do it in the same way as the Pager examples? Have type KeybindingType string, use that for all bindings, and then use JSONSchemaExtend to attach the examples? I'm not sure it's important enough though.

[karimkhaleel]

Yeah, that would also work. We would just have to find all the places these keybinds go and remember to cast them to strings. Is that a trade off we would be willing to take?

[stefanhaller]

Finding them is not the issue, the compiler will tell us. Whether it's worth it, I'm not sure; maybe not.

[karimkhaleel]

I'm sorry for the delay, I kind of lost track of who's waiting for whom here. 😅 Do I understand right that you consider this ready from your side?

I pushed one small commit to fix two comments. Nothing to do with your changes, this was already wrong on master, but we might include it here while we're at it.

I think it would make sense to squash all commits into one, could you please do that and rebase on master?

I have a few more minor things below, but then I think we're good to go. This is looking really good now.

No worries. I was traveling this last week so I wasn't able to work on this.

I experimented with the jsonschema library a bit more and found that we can use the JSONSchemaExtend function to get some of the more tricky constructs into the schema (example in cf48f4d). I'm not sure it's that developer friendly though, especially when we have to deal with primitives.

Apologies for not looking over the code as much as I should have. I wrote a script to rip them from the config that Emily wrote and assumed that they were fully vetted already.

Pushed the relevant fixes and rebased, but did not squash yet. The commit using JSONSchemaExtend is just an example of what we can do. We can just as easily use it to define oneOf or anyOf constraints.

[stefanhaller]

The commit using JSONSchemaExtend is just an example of what we can do. We can just as easily use it to define oneOf or anyOf constraints.

What would be examples of where we would use oneOf or anyOf?

[stefanhaller]

Color could be a type we could set up like this with oneOf

Right. Personally I'd also be ok with leaving it as "string" for now, and maybe consider refining it later, if at all.

[stefanhaller]

Looking at the generated schema again in more detail, there's something funny about the defaults: you have them on the leaf nodes as expected, but then there's also a "default" entry on each struct, which doesn't seem to make sense to me. It seems that this diff fixes it:

diff --git a/pkg/jsonschema/generate.go b/pkg/jsonschema/generate.go
index d1d69625b..38b582844 100644
--- a/pkg/jsonschema/generate.go
+++ b/pkg/jsonschema/generate.go
@@ -65,9 +65,7 @@ func setDefaultVals(defaults any, schema *jsonschema.Schema) {
 
 		if isStruct(value) {
 			setDefaultVals(value, subSchema)
-		}
-
-		if !isZeroValue(value) {
+		} else if !isZeroValue(value) {
 			subSchema.Default = value
 		}
 	}

[karimkhaleel]

Looking at the generated schema again in more detail, there's something funny about the defaults: you have them on the leaf nodes as expected, but then there's also a "default" entry on each struct, which doesn't seem to make sense to me. It seems that this diff fixes it:

You're right. Good catch.

[karimkhaleel]

Just moved the schema extension function closer to the PagerType alias.

[stefanhaller]

@karimkhaleel Do I understand it right that this is ready from your side?

I just had a look again, and it looks good to me; I'd still ask you to squash most of the commits though. I think we should end up with two or at most three commits in this branch.

[karimkhaleel]

It's all done from my side. Was just waiting for any extra input about where to put the JSONSchemaExtend functions, and thought the commit history might be useful there. Just squashed and rebased everything. Thank you for your time and patience @stefanhaller! I have learned so much from doing this.

[karimkhaleel]

Just made one last tiny change to the commit history though. Made the addition of the forked invopop/jsonschema a separate commit.

[stefanhaller]

I'm a bit torn about that last change. On the one hand it's nice to separate the two changes so that the second commit becomes much clearer; on the other hand, calling go mod tidy on the first commit removes the added dependency again because it's unused. I'd still lean towards keeping it separated though, hopefully that problem doesn't matter in practice.

Regarding the PR as a whole: I'm happy with this, great work! I'd be willing to merge it in this state, but @jesseduffield had some concern about why we need to fork, so I'd wait for his approval on that question. (For reference: the discussion about why we need to fork starts here.)

My questions about the fork:

• I had a quick look at the upstream repo, and I didn't see an issue or PR there. It would be nice to contribute the change upstream so that we can eventually move back to the official repo.
• Assuming the change doesn't get accepted upstream, do you have a plan how to track upstream changes? Like, rebase your fork onto upstream occasionally? It would be good to have a clear strategy here to avoid falling behind any upstream development.

[karimkhaleel]

About the fork:
I just raised an issue and submitted a PR for the changes I made.

invopop/jsonschema#116
invopop/jsonschema#117

Assuming the change doesn't get accepted we will have two options:

1. Rebasing onto upstream occasionally.
2. Rewriting setDefaultVals in jsonschema/generate.go to use the tag values of the UserConfig instead of the field names, and making a note to not use functions like KeyNamer in the future without also reflecting those changes in the setDefaultVals function.

[jesseduffield]

LGTM! (as discussed in the town hall meeting, the fork adds functionality that is of general use and not specific to lazygit so there's a good chance they'll eventually be merged, so using a fork is fine)

[stefanhaller]

@karimkhaleel So we finally got approval for this and are ready to merge. Sorry that it took so long.

Thanks for the great work, this is a big improvement! Looking forward to your next contribution. 😄

We now need a PR on https://github.com/SchemaStore/schemastore to change the URI to point to ours, and to remove the manually uploaded schema there. Would you be up for doing that too?

[stefanhaller]

@jesseduffield Something failed with uploading the coverage report to codacy. Would you have time to look into that? Since this seems unrelated to this branch, I'm inclined to merge anyway.

[jesseduffield]

@stefanhaller yep feel free to ignore that error. Looks like that secret codacy token isn't defined in forks. I'll look into a fix

[karimkhaleel]

We now need a PR on https://github.com/SchemaStore/schemastore to change the URI to point to ours, and to remove the manually uploaded schema there. Would you be up for doing that too?

I would be up for submitting a PR to schemastore. We just want it to point to the master blob config on our repo right?

This is what I would be submitting to them: SchemaStore/schemastore@master...karimkhaleel:schemastore:update-lazygit

Should I submit the PR or should I wait before this branch is merged to master?

[stefanhaller]

We just want it to point to the master blob config on our repo right?

Yes, that's right.

Should I submit the PR or should I wait before this branch is merged to master?

It's merged, go ahead!

[stefanhaller]

@stefanhaller yep feel free to ignore that error. Looks like that secret codacy token isn't defined in forks. I'll look into a fix

I found this post which looks relevant.