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.

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

Generate UserConfig struct from JSON schema #2984

Closed
jesseduffield opened this issue Sep 4, 2023 · 14 comments
Closed

Generate UserConfig struct from JSON schema #2984

jesseduffield opened this issue Sep 4, 2023 · 14 comments
Labels
enhancement New feature or request

Comments

@jesseduffield
Copy link
Owner

jesseduffield commented Sep 4, 2023

Is your feature request related to a problem? Please describe.
We currently have a yaml schema which provides type safety for users when modifying their yaml config.

But, this schema needs to be manually kept in-sync with our UserConfig struct (defined here).

We need a way to automatically keep the schema in-sync with the config.

Describe the solution you'd like
There are two ways of going about this:

I haven't spent much time looking into these alternatives. The generate package has more stars than the jsonschema-go package but the jsonschema-go package was more recently updated.

I think we should use the schema as the source of truth, because:

  • shoving descriptions into go's struct tags sounds annoying
  • we may want to produce other outputs from the schema such as documentation (i.e. replacing our current Config.md file's 'defaults' section)

So I say we do that but I'm open to arguments that say we should let the go struct be the source of truth.

Assuming that we decide to use the schema as the source of truth, we'll end up with the following tasks:

  • define the schema in our codebase
  • add a script to generate the UserConfig struct from the schema
  • add a step to CI that ensures that if you run the above script, we don't get any changes (meaning the dev has ensured that the two are kept in sync)
  • add a comment to the start of the UserConfig file telling the user that the struct is auto-generated and so shouldn't be edited directly, and pointing the user to the schema file and the generate script.
  • add a script to deploy the schema
  • add a step to CI for when a release tag is deployed which runs the deploy script
@jesseduffield jesseduffield added the enhancement New feature or request label Sep 4, 2023
@jesseduffield jesseduffield changed the title Auto-generate config's yaml schema Generate UserConfig struct from JSON schema Sep 4, 2023
@jesseduffield jesseduffield mentioned this issue Sep 4, 2023
6 tasks
@stefanhaller
Copy link
Collaborator

I'm very skeptical about a-h/generate; it's probably good for very simple things, but for anything more involved (e.g. oneOf, or if/then constructs) I'm not sure how it would handle those, and from a cursory glance it looks like it doesn't.

Re descriptions in struct tags: if we use invopop/jsonschema, it provides an option to take descriptions from regular go comments, which looks very useful.

Re producing other outputs from the schema: we could do this no matter whether we generate the structs from the schema, or the schema from the structs, no?

@jesseduffield
Copy link
Owner Author

Ah very nice. Okay let's go with that approach

@jesseduffield
Copy link
Owner Author

@karimkhaleel let me know if you'd be interested in tackling this (if not all good we can find something else)

@stefanhaller
Copy link
Collaborator

As far as I understood, @EmilyGraceSeville7cf was very interested in working on this, see #2859. Haven't heard from her in a while though.

@karimkhaleel
Copy link
Contributor

As far as I understood, @EmilyGraceSeville7cf was very interested in working on this, see #2859. Haven't heard from her in a while though.

Yeah, I wouldn't want to invalidate the work someone else already put in. But sounds like an interesting problem. Would be willing to take a look if @EmilyGraceSeville7cf is no longer interested. Otherwise I would be open to looking at some other issues.

@jesseduffield
Copy link
Owner Author

@karimkhaleel if you're still keen on this issue then go for it :)

@karimkhaleel
Copy link
Contributor

I messed around with that invopop/jsonschema for a good while. Wanted to add the default values to the jsonschema output. Got it to get comments too.

#3039

image

image

What are we going to do with the output? Push it to SchemaStore? What about versioning?

@jesseduffield
Copy link
Owner Author

Nice work! Yes, we want to push the output to SchemaStore. We can have CI do this when deploying a release.

There was some discussion re: versioning in #2859.

I'm thinking we start without versioning and see how that goes. Right now we have a static schema that isn't being updated and so is getting stale over time so having a rolling json schema is an improvement over that.

@jesseduffield
Copy link
Owner Author

If it's alright with you @karimkhaleel I'm gonna go through pkg/config/user_config.go and add some better comments. I'll raise a separate PR for that

@jesseduffield
Copy link
Owner Author

See #3040

@jesseduffield
Copy link
Owner Author

That PR is just for having explanations of things in the one place (there were a few things that had no explanation whatsoever): it doesn't attempt to make use of any json schema stuff like specifying enums in struct tags: I will leave that to you @karimkhaleel :)

Also I noticed that because we're using yaml tags, the json schema package is ignoring them meaning we don't get our CamelCase keys.

If you go:

diff --git a/scripts/jsonschema/main.go b/scripts/jsonschema/main.go
index 41d7b80f8..b44268cb3 100644
--- a/scripts/jsonschema/main.go
+++ b/scripts/jsonschema/main.go
@@ -23,6 +23,7 @@ func main() {
 func CustomReflect(v *config.UserConfig) *jsonschema.Schema {
 	defaultConfig := config.GetDefaultConfig()
 	r := new(jsonschema.Reflector)
+	r.FieldNameTag = "yaml"
 	if err := r.AddGoComments("github.com/jesseduffield/lazygit", "./"); err != nil {
 		panic(err)
 	}
@@ -72,6 +73,10 @@ func setDefaultVals(defaults interface{}, schema *jsonschema.Schema) {
 			continue
 		}
 
+		if !ok {
+			continue
+		}
+
 		property.Default = value.Interface()
 	}
 }

That helps, but it means it no longer shows default values for some reason. We should consider switching to use json tags instead. There's discussion of that here: invopop/jsonschema#28

@stefanhaller
Copy link
Collaborator

No, we don't want to push it to SchemaStore. We should commit it to our own repo, and once we have a version that works, we should make a PR on SchemaStore to change its URI to point to our master version. From then on we don't have to touch SchemaStore any more, and there's no need to "deploy" anything when making a release.

The "downside" (if it is one) is that users of the last release already get to use the schema of master, which might not match. This is only a problem if we delete or rename config entries, not if we add new ones. And since the editor that opens the user's config.yml doesn't know which version the user is using anyway, we don't have a way to use the right schema anyway, so might as well always use master.

@jesseduffield
Copy link
Owner Author

jesseduffield commented Sep 30, 2023

Ah I didn't realise that was an option. Yep, looks like it's common for people to do that.

It would be nice if there was some way to have a url saying 'latest vX.XX tag' so that we could make it release based but given that's not an option I prefer using the master branch compared to raising a PR for a new version on each release.

@stefanhaller
Copy link
Collaborator

Implemented in #3039.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants