config-schema: allow arrays of strings for ui.editor and ui.diff.tool
#5651
Conversation
| @@ -140,7 +140,10 @@ | |||
| "default": "color-words" | |||
| }, | |||
| "tool": { | |||
| "type": "string", | |||
There was a problem hiding this comment.
This seems to work, thanks!
Could you do the same for ui.pager? It actually also accepts a third config (an object with command and env fields; I'm not sure whether or not diff-tool and editor accept these), but we could omit that for now.
There was a problem hiding this comment.
Done, although I'm a bit iffy about the command and env fields. From looking at CommandNameAndArgs in cli/src/config.rs I think we can have either a string or array once again for command. env is just a hash of strings, which I'm assuming maps to a Table in TOML and an object in json-schema.
It might be nice to have a test that checks the schema actually validates some reasonable example config. Do you think that's worth doing in this PR, or should I file an issue for it?
There was a problem hiding this comment.
Thank you!
I thought you would just make the pager variable the same as the others. I imagine few people outside jj's builtin config use the object notation.
I think what you have is OK. (Very optionally) you could probably make it better by setting up a schema for the pager object notation in the definitions of the schema and using oneOf, there are a few examples of that inside the schema.
It might be nice to have a test that checks the schema actually validates some reasonable example config. Do you think that's worth doing in this PR, or should I file an issue for it?
That would be wonderful, but I think it can be a separate PR.
steadmon
force-pushed
the
push-vyvlpokvlzoy
branch
2 times, most recently
from
a80da3a to
51384ba
Compare
steadmon
force-pushed
the
push-vyvlpokvlzoy
branch
from
51384ba to
5b4e1e2
Compare
| @@ -140,7 +140,10 @@ | |||
| "default": "color-words" | |||
| }, | |||
| "tool": { | |||
| "type": "string", | |||
There was a problem hiding this comment.
Thank you!
I thought you would just make the pager variable the same as the others. I imagine few people outside jj's builtin config use the object notation.
I think what you have is OK. (Very optionally) you could probably make it better by setting up a schema for the pager object notation in the definitions of the schema and using oneOf, there are a few examples of that inside the schema.
It might be nice to have a test that checks the schema actually validates some reasonable example config. Do you think that's worth doing in this PR, or should I file an issue for it?
That would be wonderful, but I think it can be a separate PR.
Fixes #5617
Checklist
If applicable:
CHANGELOG.md