fix issue where database name was not completed when creating alias #218
Conversation
🦋 Changeset detectedLatest commit: 9775ac5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
OskarDamkjaer
force-pushed
the
missing_db_name_completion
branch
from
d31f640
to
d232350
Compare
|
@ncordon ah of course, I was wrongly thinking the whitespace was not included in the token numbers. We could just check that the last token is |
ncordon
force-pushed
the
missing_db_name_completion
branch
from
245b807
to
8360dc6
Compare
| aliasName: | ||
| symbolicAliasNameOrParameter; | ||
|
|
||
| databaseName: | ||
| symbolicAliasNameOrParameter; | ||
|
|
There was a problem hiding this comment.
These are the real changes to the grammar, the rest are just bringing it up to date
| aliasName: | ||
| symbolicAliasNameOrParameter; | ||
|
|
||
| databaseName: | ||
| symbolicAliasNameOrParameter; |
There was a problem hiding this comment.
The change I was expecting was to separate into:
createAliasOrDatabaseName:
symbolicAliasNameOrParameter;
ReferenceAliasorDatabaseName:
symbolicAliasNameOrParameter;
Because then we could use ReferenceAliasorDatabaseName instead of RULE_symbolicAliasName in the preferredRules and wouldn't have to check the tree if it's a rule that creates or references an alias/database. It could still be worth splitting database & aliases as well and this change also solves the problem, so I'm fine doing it this way if you prefer
There was a problem hiding this comment.
Yeah I agree that would be nicer for us but it could make it more difficult to enforce for engineers working in the database because it obfuscates the grammar a step further.
I'm happy to keep evolving the grammar, but we should make it gradually because pushing any change is a pain in the monorepo. Out of curiosity I completely tried to split database completions from alias completions here: 245b807 and it was super painful, so I decided to revert it 😬
There was a problem hiding this comment.
alright, good to hear it was considered!
| // For `CREATE ALIAS $1 FOR DATABASE $2` | ||
| // Should not suggest $1 but should suggest for $2 | ||
| // Ideally the grammar would separate declaring and referencing database names | ||
| // relying on the startTokenIndex is brittle as it will change as the grammar changes |
There was a problem hiding this comment.
This comment is a little out of date, how about something like this instead
| // For `CREATE ALIAS $1 FOR DATABASE $2` | |
| // Should not suggest $1 but should suggest for $2 | |
| // Ideally the grammar would separate declaring and referencing database names | |
| // relying on the startTokenIndex is brittle as it will change as the grammar changes | |
| // For `CREATE ALIAS $1 FOR DATABASE $2` | |
| // Should not suggest $1 but should suggest for $2 | |
| // so we return base suggestions if we're at the `aliasName` rule |
|
|
||
| test('Correctly completes database name even in a create alias statement including extra of spaces', () => { | ||
| testCompletions({ | ||
| query: 'CREATE ALIAS foo FOR DATABASE ', |
There was a problem hiding this comment.
I've added this extra test, which was the problematic bit in the previous solution
| @@ -617,22 +617,29 @@ function completeAliasName({ | |||
| } | |||
|
|
|||
| // parameters are valid values in all cases of symbolicAliasName | |||
| const baseSuggestions = parameterCompletions( | |||
| const parameterSuggestions = parameterCompletions( | |||
There was a problem hiding this comment.
I think renaming this makes the code read clearer because you know at any point what baseSuggestions contain
There was a problem hiding this comment.
makes sense 👍
|
Looking nice! Do we want a changeset for this? |
The database name wasn't completed in the
CREATE ALIAS foo FOR DATABASEstatement as it was caught in the logic for not suggesting old names when creating new ones. I'm uncertain if there's a more sustainable way to catch this without updating the grammar