-
Notifications
You must be signed in to change notification settings - Fork 202
Add experimental support for Jump-to-def and Find-references #337
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small changes, mostly nits. Apologies, I don't have time to run this locally right now.
* the default CLI search path is used. | ||
* @returns A list of query files found | ||
*/ | ||
resolveQueriesInSuite(suite: string, additionalPacks: string[], searchPath?: string[]): Promise<string[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the suite
param in the tsdoc
args.push('--search-path', path.join(...searchPath)); | ||
} | ||
args.push(suite); | ||
return this.runJsonCodeQlCliCommand<string[]>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you mark this function as async
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fair question, and one which applies to a bunch of other Promise
-returning functions in this file, but I think that I like the existing convention that we mark a function as async
if it needs to be, i.e., actually uses await
, and otherwise let its return type being Promise
being a sufficient signal that what it returns isn't immediately a value but a future.
* "codeQl.experimentalFeatures" directly in their vscode settings | ||
* json file. | ||
*/ | ||
export const EXPERIMENTAL_FEATURES_SETTING = new Setting('experimentalFeatures', ROOT_SETTING); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. I like Easter eggs. :)
const SELECT_QUERY_NAME = "#select"; | ||
|
||
enum KeyType { | ||
DefinitionQuery, ReferenceQuery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For easier debugging, I like to do:
DefinitionQuery = 'DefinitionQuery',
ReferenceQuery = 'ReferenceQuery'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, can do. Iirc with ts enums, you do have maps in both directions (e.g. KeyType.ReferenceQuery = 1
and KeyType[1] = "ReferenceQuery"
) but I can understand not wanting to have the extra step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. I haven't tried this out yet, so can't say if it works.
This adds the ability to use vscode command palette commands
Go To Definition
andGo To References
in source archive files for which there are appropriately tagged queries in the appropriate qlpack. The qlpack is inferred from the metadata of the database the source archive came from, in more or less the same way that quick query does.This ability is gated for the time being by a hidden feature flag in the user's vscode settings. Internal users can add
to their settings json file.
An example of "appropriately tagged queries" for C++ is added by github/codeql#3308.
Checklist
@github/product-docs-dsp
now is a great time to test-drive this, but it can be shipped as-is without documentation, because users won't be able to accidentally stumble on it undocumented.