mfontanini / presenterm

A markdown terminal slideshow tool
https://mfontanini.github.io/presenterm/
BSD 2-Clause "Simplified" License
1.33k stars 32 forks source link

feat(config): add JSON schema for validation,completion,documentation #228

Closed mikavilpas closed 7 months ago

mikavilpas commented 7 months ago

Help users write and maintain their configuration

This change introduces a JSON schema for the configuration file. This schema is used to validate the configuration file and provide feedback to the user if the configuration file is invalid.

https://github.com/mfontanini/presenterm/assets/300791/9b9e7c45-6f2f-4a45-9806-60a8413c2185

To use this schema, the schema must be referenced in the yaml configuration file like this:

# yaml-language-server: $schema=./schema.json

See the documentation for this feature here: https://github.com/redhat-developer/yaml-language-server?tab=readme-ov-file#using-inlined-schema

This configures the yaml-language-server to use the schema.json file to validate the configuration file. The language server is well supported in editors such as

Supported features

Testing the feature

To test this feature, you can do the following steps:

mikavilpas commented 7 months ago

Just realized one setting defaulted to a different value in the schema than what the actual default is. This is now fixed.

mfontanini commented 7 months ago

Love the goal of this! I wonder if this could be dynamically generated by using something like schemars. e.g. run presenterm --generate-config-json-schema or something and spit it to stdout.

My concern is this will eventually drift off from the config when new parameters are added and/or it is a bit harder for people to use given this is standalone file in the repo that they don't get if they download a binary artifact.

mikavilpas commented 7 months ago

Oh I see your point, and I think you are 100% correct.

I'll see if I can build the schema automatically based on the rust types 🤔 👍🏻

mikavilpas commented 7 months ago

question: It looks like AsciiBlocks is the default image rendering protocol and it cannot be configured in the configuration explicitly. Is this correct? I suppose it makes sense, but wanted to make sure.

mfontanini commented 7 months ago

It should be configurable. This works for me, as in, starting presenterm with this config will complain because I don't have sixel enabled during compile time:

defaults:
  image_protocol: sixel
mikavilpas commented 7 months ago

Ok, I have a working proof of concept available for review and feedback now.

This version is able to generate the schema using schemars. The schema seems to work and should now be easy to keep in sync with the code.

Open question: how should you think the build system should generate this schema?

Do you have an idea how things like this are usually managed in rust projects?

mfontanini commented 7 months ago

I am not sure if this is now a runtime dependency which might have an effect on the program's size

I wouldn't worry about this. I'm generally cautious about adding dependencies but this is only pulling a couple and I think it's for a good cause so I don't mind the extra binary size.

What I was thinking though, is not to have the schema file in the repo at all and let people generate it manually if they feel like using it. e.g. if you're interested in using a yaml lsp, you can run presenterm --generate-config-file-scema > /some/path/i/like and then use the yaml lsp directive in your config file to point to this. Alternatively, we could at least add a step in the CI that verifies that generating the file spits out an identical file to whatever schema file we ship in the repo. Thoughts? I don't know if it's worth shipping it in the repo if people can generate it on their own.

mikavilpas commented 7 months ago

I wouldn't worry about this. I'm generally cautious about adding dependencies but this is only pulling a couple and I think it's for a good cause so I don't mind the extra binary size.

Okay 👍🏻 I was thinking it could also be a separate cargo project/script in the repo in order to clearly limit the dependencies out of the production build - but this is certainly simpler.

What I was thinking though, is not to have the schema file in the repo at all and let people generate it manually if they feel like using it. e.g. if you're interested in using a yaml lsp, you can run presenterm --generate-config-file-scema > /some/path/i/like and then use the yaml lsp directive in your config file to point to this. Alternatively, we could at least add a step in the CI that verifies that generating the file spits out an identical file to whatever schema file we ship in the repo. Thoughts? I don't know if it's worth shipping it in the repo if people can generate it on their own.

I would go for the second option (shipping a file), but I guess a switch for creating the schema might also be a good option for advanced users. It's almost no extra work to do it as you said you are fine with having the schema related dependencies being shipped.

I forgot to mention this, but here's my initial idea for how the schema could be built and used:

This is essentially the setup that the lazygit project uses for their configuration (see their docs https://github.com/jesseduffield/lazygit/blob/master/docs/Config.md?plain=1#L19)

mikavilpas commented 7 months ago

Just added a command for creating the schema:

image

Let me know how you think the schema should be generated and if it should be added to the config.

I will also clean up the commits when everything is ready - wanted to keep the history super easy to understand for easier discussion.

mfontanini commented 7 months ago

Sorry, I've been busy and had no time to look into this.

I forgot to mention this, but here's my initial idea for how the schema could be built and used:

All of that sounds great! We can add a step in the CI in a separate PR but I think all of that sounds great :ok_hand:

mikavilpas commented 7 months ago

Ok I think things are looking good now. Just rebased to the main branch and cleaned the commit history.

I'll see if I can work on the CI part next, but as far as I'm concerned, this PR is ready to be merged.

mfontanini commented 7 months ago

Thanks again!

mfontanini commented 7 months ago

I can do the CI part later today, no worries :ok_hand:

mfontanini commented 7 months ago

I think I'm going to rename the schema.json file to config-file-schema.json. I imagine we may want a schema for the theme file as well so this future proofs this. Will ping you in the next PR.