Fix naming and availability in command palette of various commands#377
Fix naming and availability in command palette of various commands#377jcreedcmu merged 4 commits intogithub:masterfrom
Conversation
|
(Sorry, a few drive-by questions!)
|
This corrects what is an unfortunately common accidental antipattern, where creating a command meant just to be the handler of a user interface button ends up in the command palette unless you explicitly set `"when": "false"` in the command palette section of the configuration. Also enforce the naming convention that commands prefixed with `codeQLDatabases.` are those meant for the databases panel only, while prefixing `codeQL.` means that it's meant to be directly accessible through the command palette.
|
Oh, good catch! I had misunderstood the problem. These unprefixed commands were not supposed to be in the command palette at all, but just hooked up to those database view buttons. The command |
| "onCommand:codeQL.chooseDatabaseFolder", | ||
| "onCommand:codeQL.chooseDatabaseArchive", | ||
| "onCommand:codeQL.chooseDatabaseInternet", | ||
| "onCommand:codeQLDatabases.chooseDatabaseFolder", |
There was a problem hiding this comment.
I had thought the difference between the codeQL namespace and the codeQLDatabases namespace was that the latter accepted an existing database as an argument (and so was only applicable in the databases view). Whereas the former could be invoked in any context.
I see you're disabling them as global commands, so maybe that's what we want.
There was a problem hiding this comment.
Happy to be persuaded about choice of convention, but my intent so far about the codeQL$FOO.$BAR name structure (where $FOO is nonempty) was just about scoping to portions of the interface. Consider for example codeQLDatabases.sortByName which does not take a database as an argument, and things like codeQLQueryHistory.openQuery which are relevant to user events taking place in the query history view, but don't imply any particular constraints about the types of those handlers.
| "when": "false" | ||
| }, | ||
| { | ||
| "command": "codeQLDatabases.chooseDatabaseFolder", |
There was a problem hiding this comment.
Do we want to disable these as global commands?
There was a problem hiding this comment.
We definitely do want to disable these commands if we want to consistently maintain the pattern that the hover text over the buttons is not be prefixed, and the command palette command names is prefixed with CodeQL: . And I think both of those patterns are desirable.
However, we totally could also have command-palette-visible commands (which would be named, like, codeQL.chooseDatabaseFolder) which do the same thing.
There was a problem hiding this comment.
That makes sense. Maybe we don't want to pollute the global scope here with commands that are so tightly related to the databases view (I'm contradicting what I said earlier). So, what you have right now is fine.
|
Yes, this is inconsistent. Maybe the best solution is to simply remove |
As commented above, I don't think we need to remove it, but I could make the naming of the internal command more consistent, and add command palette versions of the other two, just in case people are exploring around the command palette and/or prefer to do things with the keyboard instead of clicking on icons with the mouse. Commit incoming shortly... |
|
Either way is fine: removing the palette command, or adding the 2 extra palette commands available consistently. |
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Nice. Thanks for fixing.
jcreedcmu
changed the title
Attempt to enforce some regularity in how we name commands, and fix one command that was showing up improperly in the command palette.
3 participants
No description provided.