[WIP] Add option documentation to :set-option, :get-option.

[askreet]

Closes #5938. Oops, I mean closes #5939.

Looking for some early feedback on approach here. The idea is that we move the options from configuration.md into a structured format that can be used both by docgen and the help built into the editor.

When combined with #5966 (which suppresses autocomplete for the second argument) this is the user experience:

If this general direction gets a few 👍, I would do the following before merge:

• Finish importing all the options.
• Update configuration.md to be some sort of template that can pull sections of options from runtime_options::Options and build the tables dynamically.

Some open questions I have:

• The current approach is hacked in for a subset of specific commands. More generally, we want the ability to defer documentation optionally to a TypedCommand, perhaps by adding a doc_fn: Option<Fn...> to TypedCommand?
• Should the option-specific documentation be in addition to the command documentation, perhaps separate by a === line, instead of replacing it?
• My Lazy<Result<...>> for the reference to runtime_options::Options is defensive against the runtime directory not having help/options.toml. Is this a good approach? Alternatives?
  • One option would just be baking the options into a const static instead of using toml. Thoughts?

Potential future roadmap:

• Auto-completers with more context that would allow us to auto-complete one-of validations in the editor (bufferline is a prime example).
• Color or popups when the pending value is not correct.
• More specific validators: numeric ranges, regular expressions, filesystem paths?

[the-mikedavis]

Why store the options in runtime/ as TOML? We know all options at compilation time so it should be possible to describe them using types and/or constants

[askreet]

Why store the options in runtime/ as TOML? We know all options at compilation time so it should be possible to describe them using types and/or constants

I had the same question (listed above), any tradeoffs to consider here? I look at something like languages.toml and some of the other files in runtime and wonder when each of these options is preferred? Presumably it just balloons the binary size a bit. Any other downsides?

If not, I'll move it into a constant -- seems a lot easier to work with (e.g., not having to worry about whether it loads).

[the-mikedavis]

The configuration in languages.toml is exposed as TOML so that you can override or extend it without recompiling the editor. That wouldn't help for options though since you would need to edit/recompile the editor anyway to take advantage of a new option.

A large binary size was a problem in the past but that was mostly about tree-sitter parsers being built in to the Helix binary (see #432). I don't think the binary size change would be very noticeable from adding option docs

[pascalkuthe]

While I like the general direction of providing a better user experience I would prefer if we could auto generate the data from doc comments (and update those to match the documentation once).

I have been thinking that we could use the schemars crate here. It's designed around automatically providing json schemas for serde deserialized structs. This would also solve #3901 and you can extract doc comments (or really any data about config) from this quite easily. For example:

use schemars::gen::SchemaGenerator;
use schemars::JsonSchema;

#[derive(JsonSchema)]
/// Hello Foo
struct Foo {
    /// Hello Bar
    pub bar: String,
}

#[test]
fn test_schmema() {
    let mut schema = Foo::json_schema(&mut SchemaGenerator::default()).into_object();
    assert_eq!(schema.metadata().description.as_deref(), Some("Hello Foo"));
    assert_eq!(
        schema.object().properties["bar"]
            .clone()
            .into_object()
            .metadata()
            .description
            .as_deref(),
        Some("Hello Bar")
    );
}

Just like serde schemars provides powerful capabilities around customizing the derived functionality so it would be easy to customize just like serde. In general I think it makes more sense to provide a json schema for the documentation rather than come-up with our own format here. It also doesn't add too much complexity to the code.

[askreet]

While I like the general direction of providing a better user experience I would prefer if we could auto generate the data from doc comments (and update those to match the documentation once).

Yeah! I was looking for something to simply give me the doc comments but it all seemed really complicated. Didn't consider extracting a schema first and traversing that. I really like this idea, let me spend some time on it over the coming days/weeks.

[askreet]

@pascalkuthe I managed to get it working using the JsonSchema trait -- it's very cool. The code is super rough right now, and doesn't hit a ton of cases, but works for all top level booleans and numerics, as well as enums with simple variants (e.g., bufferline). The schemars crate doesn't offer a lot of flexibility in traversing it's graph so the code is full of boilerplate and unwraps. My thoughts on moving forward:

• If we write an algorithm that handles all the current shapes of our config types, are we concerned about the maintainability of adding differently-shaped types? For example, the current implementation doesn't know what to do with idle-timeout which has a structure with two fields, and some custom serialization/deserialization logic. I'm pretty sure the JsonSchema for it is just wrong.
• Assuming we move forward, how defensive should this code be? If we run from_config in a test case (as I have now) it should be impossible for this to fail at runtime if that test has passed. Is that good enough, or should I put a lot of effort into converting all the unwrap cases into errors for example?

Overall I'm a bit torn, I love the code being the source of truth, but I wouldn't want it to be harder to expand the configuration structures in the future, especially if someone tries to do that and then has to look at all the struct->JsonSchema->magic->runtime_options::Options heroics I may have to do to get this to where I want it.

[pascalkuthe]

@pascalkuthe I managed to get it working using the JsonSchema trait -- it's very cool. The code is super rough right now, and doesn't hit a ton of cases, but works for all top level booleans and numerics, as well as enums with simple variants (e.g., bufferline). The schemars crate doesn't offer a lot of flexibility in traversing it's graph so the code is full of boilerplate and unwraps. My thoughts on moving forward:

* If we write an algorithm that handles all the current shapes of our config types, are we concerned about the maintainability of adding differently-shaped types? For example, the current implementation doesn't know what to do with `idle-timeout` which has a structure with two fields, and some custom serialization/deserialization logic. I'm pretty sure the JsonSchema for it is just _wrong_.

* Assuming we move forward, how defensive should this code be? If we run `from_config` in a test case (as I have now) it should be impossible for this to fail at runtime if that test has passed. Is that good enough, or should I put a lot of effort into converting all the unwrap cases into errors for example?

Overall I'm a bit torn, I love the code being the source of truth, but I wouldn't want it to be harder to expand the configuration structures in the future, especially if someone tries to do that and then has to look at all the struct->JsonSchema->magic->runtime_options::Options heroics I may have to do to get this to where I want it.

I would want to keep it simple too so we shouldn't write some complex magic to extract the description.

That being said, I think schemars is actually build to generate schemas for custom destabilization logic. Just like serde allows you to specify a custom deserialize using #[serde(with for a field or just implement Deserialize yourself you can use #[schema(with or just impl JsonSchema yourself. So you really get the best of both worlds. An Automatically generated schema for most cases avoiding duplication and in the edgecases where deriving doesn't quite cover it you can specify the schema manually (which would be required with a manual approach anyway)

Assuming we move forward, how defensive should this code be? If we run from_config in a test case (as I have now) it should be impossible for this to fail at runtime if that test has passed. Is that good enough, or should I put a lot of effort into converting all the unwrap cases into errors for example?

If I understand what you mean here correctly than I think unwrapping is fine since the schema is essentially hardcoded. That said you shouldn't need unwrap for the mostpar. schemars provides function to lazily create fields so you can use schema.metadata() instead of schema.metadata. My example doesn't need any unwraps.

[askreet]

... you can use #[schema(with or just impl JsonSchema yourself.

Ah, good idea. I haven't looked to see what annotations they support but you're right I could implement JsonSchema myself rather than juggle the bits on the scanning side. I'll find some time in the coming days to keep poking at this, thanks for the thoughts. 👍🏼

[askreet]

@pascalkuthe I spent a bit more time on this trying to adapt JsonSchema to some of the special-case config options that exist in the project and I'm about ready to give up and pivot to something more structured (such as a trait implementation we can create for each runtime-configurable pile of settings). These edge cases (like auto-pairs accepting boolean values, but not really being a bool) are really rough to work with within the JsonSchema hierarchies and produce piles of code as a result. I'm curious for your thoughts here on whether something simpler is a good interim step until someone with more stamina wants to chase this down?

This branch can export the full JSON schema of the config object, I'm not sure if that's useful for #3901 or if it has all the same edge cases.

[askreet]

I'm going to close this out for now. IMO, the JsonSchema stuff, while interesting, is a ton of work and potentially breaks some of the config modeling. I'm going to pivot this to a statically defined approach (as @the-mikedavis mentioned) when I have some cycles to work on it.

[goyalyashpal]

@askreet did u continue this in some other pr?

[askreet]

@goyalyashpal I haven't. Feel free to pick it up if interested, there's some generally useful bits here for either approach.