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

Better validation on binding settings #7

Open
stevenguh opened this issue Aug 1, 2020 · 8 comments
Open

Better validation on binding settings #7

stevenguh opened this issue Aug 1, 2020 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@stevenguh
Copy link
Member

This is split from VSpaceCode/VSpaceCode#78 and to track the work to have better error messages when which key binding json is incorrect.

@stevenguh
Copy link
Member Author

stevenguh commented Aug 9, 2020

Just jotting notes here. Thinking we can use JSON Schema with https://ajv.js.org. To handle requirements of different requirement of properties on the type name, we can use the if/then/else as described in https://json-schema.org/understanding-json-schema/reference/conditionals.html and https://ajv.js.org/keywords.html#ifthenelse

And we can use https://github.com/adobe/jsonschema2md to generate documentations

@MarcoIeni
Copy link
Member

I've used json schema before. I could write the schema if you want.

Tell me:

  • the file(s) to be validated
  • where to place the schema file

@stevenguh
Copy link
Member Author

Thank you for helping, it would be great if you can help as I am going to be busy in the next few weeks. The schemas are used to validate the bindings and bindingOverrides.

Thebindings contains an array of BindingItem(s).

BindingItem

key value
key required string
name required string
type required enum string (command, commands, bindings and transient)
command required string if type == command | (optional if type == transient && only if commands is not exists)
commands required array of strings if type == commands | (optional if type == transient && only if command is not exists)
args optional any object | array of objects if type == commands
bindings Array of BindingItem(s), required if type == bindings

The bindingOverrides contains an array of OverrideBindingItem(s).

OverrideBindingItem

key value
keys required (string | array of string)
position optional number
name optional string if keys is negative else required
type optional enum string as in BindingItem if keys is negative else required
command Same as BindingItem
commands Same as BindingItem
args Same as BindingItem
bindings Same as BindingItem

Can you put the schemas in src/scemas in a new branch? Then I can use the schema to validate finish this out.

@MarcoIeni
Copy link
Member

MarcoIeni commented Aug 21, 2020

if keys is negative else required

Do you mean position, right?

Furthermore, is args required if type == commands?

@stevenguh
Copy link
Member Author

Do you mean position, right?

Sorry, yes.

Furthermore, is args required if type == commands?

They are optional as not all commands need args

@MarcoIeni
Copy link
Member

MarcoIeni commented Aug 21, 2020

Take a look at b331678

Are the examples valid? They are validated by vscode, but I don't think the schema is that accurate at the moment and I am not a json schema expert, so feel free to edit both schemas or examples if you want.

The biggest problem at the moment is that I don't know a better way to apply the following constraint to the schemas/binding.json file, too.

Do you know how to solve this problem? If not, I will try to figure it out by myself and if I find no way I will just copy and paste.

@stevenguh
Copy link
Member Author

The examples seems to be correct, although it doesn't encompass the whole spectrum. It would be still be a great testing point. Thank you for getting the preliminary schema up. I'd suggest to change the schema for bidningOverrides and bindings to the array type. Ultimately, we only care if the array user entered is correct.

I also added a testing code for loading schema and validating with Ajv. It is very preliminary, you can run it by

npm run test-compile
node out/test/suite/schema.test.js

Also make sure you run npm install to install the dependencies.

@stevenguh
Copy link
Member Author

Kind of just jotting here. Apparently, we can add the (limited) schema into package.json so there is a prompt of possible value when people edit the settings.json
For example:

"whichkey.bindings": {
    "type": "array",
    "items": {
        "type": "object",
        "title": "Binding",
        "properties": {
            "key": {
                "type": "string",
                "description": "Key of this binding in this level"
            },
            "name": {
                "type": "string",
                "description": "The display name of this binding"
            },
            "type": {
                "type": "string",
                "enum": ["command", "commands", "transient", "conditional"],
                "description": "The type of this binding"
            }
        }
    },
    "markdownDescription": "The bindings of the which key menu",
    "default": []
}

Maybe the hack mentioned in microsoft/vscode#95270 (comment) can bypass the limited functionality. Also maybe be able to reuse across extensions like VSpaceCode and which-key

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

No branches or pull requests

2 participants