Skip to content
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

feat: named constraints + improvements #919

Merged
merged 6 commits into from Oct 28, 2021
Merged

feat: named constraints + improvements #919

merged 6 commits into from Oct 28, 2021

Conversation

Jolg42
Copy link
Member

@Jolg42 Jolg42 commented Oct 27, 2021

Closes #825

The goal of this PR is to add auto-completion for named index / constraints added in 2.29.

I also used the available feature insertText which makes it possible to display something but insert a different version which we insert in snippet mode and $0 means that the cursor will move to that position for better auto-completion experience.

      "label": "references",
      "insertText": "references: [$0]",

@Jolg42 Jolg42 added this to the 3.4.0 milestone Oct 27, 2021
@Jolg42 Jolg42 marked this pull request as ready for review October 28, 2021 08:16
@Jolg42 Jolg42 requested review from do4gr and tomhoule October 28, 2021 08:18
@Jolg42
Copy link
Member Author

Jolg42 commented Oct 28, 2021

@do4gr Could you check if I missed something here?

Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The completion utility changes are bit hard for me to navigate, maybe we can discuss that in our call this afternoon?

Copy link
Member

@do4gr do4gr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, main remaining questions are around connector aware suggestions and test expectations as per my comments.

@@ -179,7 +227,8 @@
]
},
{
"label": "@default()",
"label": "@default",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on SQL Server defaults are also named. So there they will have a map argument. I'm not sure we currently have the capability to give connector aware autocomplete?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have that, we also should adjust the map argument on primary keys, since on mysql and sqlite these are not named. Same goes for foreign keys on sqlite, we use no names there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we could build that I think using some regex then.

{ line: 34, character: 13 },
{
isIncomplete: false,
items: [fieldsProperty, nameProperty, mapProperty],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it correctly that the items are the generated suggestions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

{ line: 40, character: 33 },
{
isIncomplete: false,
items: [fieldsProperty, nameProperty, mapProperty],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are then fields still suggested if already present?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of @@unique() it's expected to have all suggestions
In the case of @@unique([email, something], ) it is indeed not expected to have all suggestions, fields should be removed though this looks like something we should do in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that @@unique(fields: [email, something], ) would then work as expected (not suggest fields)

Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a zoom call to review this, and it's indeed a big improvement, kudos!

@Jolg42 Jolg42 merged commit 3cfa1b9 into main Oct 28, 2021
@Jolg42 Jolg42 deleted the joel/named-constraints branch October 28, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Language tool support for named contraints/indexes
3 participants