-
Notifications
You must be signed in to change notification settings - Fork 47
Fix autocomplete suggesting erratically & add more suggestions #866
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
Fix autocomplete suggesting erratically & add more suggestions #866
Conversation
✅ Deploy Preview for typedb-studio ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
8d72c22 to
9cd1f7f
Compare
krishnangovindraj
left a comment
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.
quick self review
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.
This is a rather generic framework for autocomplete. I don't know its limitations.
You should not have to change this file to add new suggestions, just update typeql_suggestions.ts
|
|
||
| static fromDriver(driver: DriverState, database: string): TypeQLAutocompleteSchemaImpl | null { | ||
| function runQuery(driver: DriverState, database: string, query: string): ConceptRowAnswer[] { | ||
| // todo: Help please alex |
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.
@alexjpwalker , This is the function I don't know how to implement in the current state of studio. Can you sketch me an implementation? (or do it if you think it's faster that way).
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.
You did tell me to add a subscriber in the constructor of (schema-state-service iirc) and it works.
| return autocompletion({ override: [wrappedAutocomplete] }); | ||
| } | ||
|
|
||
| function updateSchemaFromDB(driver: DriverState, database: 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.
Actually, this is the function that needs to be called when the user wants to reload the schema
| readonly TypeQL = TypeQL; | ||
| readonly linter = otherExampleLinter; | ||
| readonly typeqlAutocompleteExtension = typeqlAutocompleteExtension; | ||
| readonly codeEditorKeymap = keymap.of([...defaultKeymap, {key: "Alt-Space", run: startCompletion, preventDefault: true}]); |
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.
I added this for mac. I couldn't get the default combination to work.
| if (isApiErrorResponse(res)) return; | ||
|
|
||
| if (res.ok.answerType == "conceptRows" && res.ok.query != null) { | ||
| (window as any)._lastQueryAnswers = res.ok.answers; |
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.
Sneak this into the global namespace for testing schema based suggestions.
| { suffixes: [ [tokens.PLAYS] ], suggestions: [ suggestRoleTypeLabelsScoped ] }, | ||
| { suffixes: [ [tokens.RELATES] ], suggestions: [ suggestRoleTypeLabelsUnscoped ] }, | ||
| ], | ||
| // TODO: ... |
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.
Anything we find missing.
778b45f to
6d40872
Compare
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.
This file will ideally go away and be replaced by the schema state built for the schema tree.
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.
Now that schema state is merged can we do this refactor?
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.
No, we need to keep it since it wraps two instances of the tree.
One that originates from the DB and one from the editor-state
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.
This file will have to be updated to use your state instead of mine.
Doesn't, since my state now wraps two instances of your state.
| return autocompletion({ override: [wrappedAutocomplete] }); | ||
| } | ||
|
|
||
| function updateSchemaFromDB(schema: Schema) { |
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.
How do I call this on schema change, Alex?
7982e8c to
6150679
Compare
6150679 to
0f6b708
Compare
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.
Now that schema state is merged can we do this refactor?
Release notes: product changes
We fix a bug where the suggestions of variables & labels were unpredictable, and implement more suggestions in an extensible, but ad-hoc way.
We enable suggestions based on a datastructure representing the schema, allowing labels to be suggested based on the schema. The schema is loaded both from the database and from the editor state.
Motivation
Better autocomplete.
Implementation
We walk up the tree till we find a node we want to do a suggestion at. We're provided with a "prefix". This is the union of the direct children of every node on the path from this suggesting node to the node being parsed*. The nodes on the path are excluded.
E.g. If we reach
ClauseMatchwhereClauseMatch { MATCH Patterns }; Pattern { (Pattern SEMICOLON)+ }on our climb,match $x isa person; $y, prefix would be[MATCH, Pattern, SEMICOLON, VAR].Ideally, we'd be able to partially generate suggestions from the grammar. Lezer provides a grammar for lezer grammar files.
Manually testing the schema-based suggestions
And this in the JS console:
_updateSchemaFromDB(_lastQueryAnswers, _lastQueryAnswers, _lastQueryAnswers)