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

fix: merge local and engines completions #1348

Closed
wants to merge 1 commit into from
Closed

Conversation

Druue
Copy link
Contributor

@Druue Druue commented Jan 6, 2023

closes #1347

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

🤖 Pull request artifacts

file commit
pr1348-prisma.vsix 45e8ba2

github-actions bot added a commit that referenced this pull request Jan 6, 2023
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

@Druue Druue changed the title merge local and engines completions fix: merge local and engines completions Jan 7, 2023
@Jolg42 Jolg42 added this to the 4.9.0 milestone Jan 9, 2023
Comment on lines +358 to +373
if (prismaFmt === undefined && local === undefined) {
return undefined
}

if (prismaFmt !== undefined && local === undefined) {
return prismaFmt
}

if (prismaFmt === undefined && local !== undefined) {
return local
}

return {
isIncomplete: false,
items: prismaFmt!.items.concat(local!.items),
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (prismaFmt === undefined && local === undefined) {
return undefined
}
if (prismaFmt !== undefined && local === undefined) {
return prismaFmt
}
if (prismaFmt === undefined && local !== undefined) {
return local
}
return {
isIncomplete: false,
items: prismaFmt!.items.concat(local!.items),
}
if (prismaFmt && local) {
return {
isIncomplete: false,
items: [...prismaFmt.items, ...local.items],
}
} else if (prismaFmt) {
return prismaFmt
} else if (local) {
return local
}
return undefined

Maybe this instead?

@Jolg42
Copy link
Member

Jolg42 commented Jan 9, 2023

It looks like the tests are failing currently, so I think we cannot merge the completions for now until we figure out how to resolve this

@Druue
Copy link
Contributor Author

Druue commented Jan 10, 2023

It looks like the tests are failing currently, so I think we cannot merge the completions for now until we figure out how to resolve this

Yeah! The tests are currently failing due to not expecting certain completions to be available but they all of a sudden are. I'd hedge that this is more than updating the expects of the test suites and rather, being more specific about when completions will be available.

As an example, looking at the test for @@index
image

@Jolg42
Copy link
Member

Jolg42 commented Jan 10, 2023

After taking another look at the test failures

for @@index([title], type: |)
Before we were suggesting

         "Gist"
         "Gin"
         "SpGist"
         "Brin"

and now only the following, which is less correct

         "Gist"
         "Gin"
         "SpGist"
         "Brin"
         "fields"
         "map"

for @@index([title(ops: |)]) (and similar):
Before we were suggesting

          "raw"

and now only raw, which is less correct

          "raw"
         "ops"
         "sort"

for @relation(onDelete: |) (and similar) :
Before we were suggesting

          "Restrict"
         "NoAction"
         "SetNull"
         "SetDefault"

and now only the referential actions, which is less correct

          "Restrict"
         "NoAction"
         "SetNull"
         "SetDefault"
         "references"
         "fields"
         "onDelete"
         "onUpdate"
         "\"\""
         "name"
         "map"

@Jolg42
Copy link
Member

Jolg42 commented Jan 10, 2023

I think we can avoid/delay this work if instead of prisma/prisma-engines#3541 we open a TypeScript PR so we can have all datasource completions on one side. It's quite easy to do, only need to change the completions.json file here

It will also have the benefit of working from the beginning of the line. (Bug in rust code is tracked here: prisma/prisma#17000)

@Druue Druue closed this Mar 16, 2023
@Druue Druue deleted the fix/merge-completions branch May 23, 2023 17:43
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.

Completion: merge between language-tools and engines
2 participants