search

SublimeText / PackageDev

Tools to ease the creation of snippets, syntax definitions, etc. for Sublime Text.
MIT License
436 stars 83 forks source link

validate built-in command metadata #192

Closed keith-hall closed 6 years ago

keith-hall commented 6 years ago

This PR ensures that when the PackageDev command_completions plugin is loaded, it will check that the metadata could be loaded successfully, and warn if it couldn't. It will also log a warning if the metadata file, which is supposed to contain built-in (core) commands only, contains a command defined in the Default package. This will allow PackageDev developers to make changes to the metadata file, restart ST and confirm (from looking at the ST console) that the file is still parse-able and doesn't contain any mistakes. It also highlights that I shouldn't have added the indentation related commands in https://github.com/SublimeText/PackageDev/pull/190, as I forgot they exist in the Default package. (For some reason they weren't showing up as completions when I was trying something out the other day, which is what inspired me to add them.)

@FichteFoll, if we don't want it to parse the metadata on plugin load, and instead defer it to later - when the file is actually read through normal use of the plugin, that's fine by me - my main goal here is to have a way to confirm that the metadata file is correct, and if it doesn't validate automatically but I can still execute the verification routine manually on demand, then that is fine by me.

Unfortunately I don't think we'll be able to set up the CI to spot check this for PRs, but if you can think of a way that'd be great! ;)

FichteFoll commented 6 years ago

I think I'd rather validate within get_builtin_commands and print warnings there, which is guarded by a lru_cache, i.e. lazily executed, with the option to run it on-demand (by importing the function from the console, for example - like I do with _check_missing).

FichteFoll commented 6 years ago

Also, I didn't check, but the reason for adding the commands to the build-ins manually might also be to provide a docstring. Though, I don't remember whether we even use that anywhere.

keith-hall commented 6 years ago

That's a good point - this seems to be the first instance of us including a Default package's command in this meta data file, which is why I figured it was not something we normally do. I also don't remember if we use the docstring anywhere, but I haven't seen it used. Maybe in the future we could add a tooltip when hovering over a .run_command( method and over the command in the keymap etc. to show the docstring.