SOQL code completion for SELECT fields #99
Conversation
* Extracts the object referenced in FROM * Supports nested SELECT statements * Generate completion item with SELECT FROM snippet only where it applies * Actual retrieval of SObject fields is responsibility of the LSP client
dobladez
requested review from
amphro,
code0100fun,
dehru,
jgellin-sf,
jonnyhork,
mysticflute,
praksb and
tmock12
as code owners
jgellin-sf
left a comment
jgellin-sf
left a comment
There was a problem hiding this comment.
Solid work here. One question about the matching algorithm.
| const selectToken = this.selectsStack.pop(); | ||
| if (selectToken) { | ||
| this.matchedSelectFroms.push({ | ||
| select: selectToken, | ||
| from: fromToken, | ||
| sobjectName: sobjectName, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Does SELECT (SELECT |) FROM X break the stack-based matching? If so, won't SELECT |, (SELECT ) FROM X not match the first SELECT with the X object? Do we need a check to make sure the SELECT and FROM tokens have the same immediate inner query ancestor?
There was a problem hiding this comment.
Good catch!. Hmm... the second case you mention seems to work fine.
On the first case, it proposes completions for X (which is wrong) but it's not "matching" (that is "pairing") of FROMs with SELECTs that's broken, the problem is on method findSObjectName() which is only looking at the matched pairs, ignoring the SELECTs remaining on the stack.
I'll try to fix it.
BTW, How about I rename matchedSelectFroms to pairedSELECT_FROMs or similar?
There was a problem hiding this comment.
I'm ambivalent on the rename. Whatever works. :)
There was a problem hiding this comment.
No, I was wrong! You are right that the matching fails on the first case!
I've replied too quickly assuming the "pairing" of the FROMs was done on exitSoqlInnerQuery (instead of exitSoqlFromExprs)... so yes, the FROM is paired with the inner SELECT on your first example.
The second example does work, but for the wrong reason ;-): The second SELECT is not recognized by the parser due to the syntax error, thus the FROM is paired with the first SELECT.
Anyway, I'll work on a fix. Thanks again!
Codecov Report
@@ Coverage Diff @@
## develop #99 +/- ##
===========================================
+ Coverage 94.56% 94.62% +0.05%
===========================================
Files 43 43
Lines 1215 1283 +68
Branches 258 283 +25
===========================================
+ Hits 1149 1214 +65
- Misses 64 67 +3
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
jgellin-sf
left a comment
jgellin-sf
left a comment
There was a problem hiding this comment.
QA passed. The only issues I found were those already identified and to be covered by W-8558994.
* model WHERE clause (#68) * Remote SOQL Errors - Language Server Part (#93) * remove code builder (#92) * Move from antlr4JS to antlr4TS (#96) * SOQL code completion for SELECT fields (#99) * publish language server 0.2.9 (#102) * Fix LSP dependency (#104) * Disabled 'Running...' button while query running (#106) * Add basic code-completion for ORDER BY and other improvements (#107) * Dehru and JG telemetry - Send GDPR clean data to telemetry when errors/unsupported syntax present (#110) * Disable WHERE in model (#113) * Bump versions for publishing (#114) * Code-completion for WHERE clause expressions (#112) Co-authored-by: Jonathan Gellin <jgellin@salesforce.com> Co-authored-by: jgellin-sf <55159130+jgellin-sf@users.noreply.github.com> Co-authored-by: Chase McCarthy <charles.mccarthy@heroku.com> Co-authored-by: Jonny Hork <jhork@salesforce.com> Co-authored-by: Fernando Dobladez <fernandodobladez@salesforce.com> Co-authored-by: Jonathan Gellin <jgellin@salesforce.com>
2 participants
What does this PR do?
Provide initial code completion with fields on SELECT clause
What issues does this PR fix or reference?
W-8432064