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(fmt): lsp hover #4923

Merged
merged 25 commits into from
Jul 10, 2024
Merged

feat(fmt): lsp hover #4923

merged 25 commits into from
Jul 10, 2024

Conversation

Druue
Copy link
Contributor

@Druue Druue commented Jun 14, 2024

image

Screen.Recording.2024-06-14.at.20.27.12.mov
Screen.Recording.2024-06-15.at.03.48.47.mov

related: https://github.com/prisma/team-orm/issues/1202
closes: prisma/language-tools#1172

Copy link

codspeed-hq bot commented Jun 14, 2024

CodSpeed Performance Report

Merging #4923 will not alter performance

Comparing feat/hover (9b731ac) with main (4b84e51)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented Jun 14, 2024

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.044MiB 2.044MiB -263.000B
Postgres (gzip) 814.784KiB 814.624KiB 164.000B
Mysql 2.013MiB 2.013MiB -255.000B
Mysql (gzip) 801.060KiB 801.406KiB -354.000B
Sqlite 1.915MiB 1.915MiB -259.000B
Sqlite (gzip) 763.623KiB 763.608KiB 15.000B

Druue added a commit to prisma/language-tools that referenced this pull request Jun 17, 2024
Druue added a commit that referenced this pull request Jun 17, 2024
Druue added a commit that referenced this pull request Jun 18, 2024
Druue added a commit that referenced this pull request Jun 19, 2024
@Druue Druue added this to the 5.17.0 milestone Jun 27, 2024
@Druue Druue marked this pull request as ready for review June 27, 2024 11:34
@Druue Druue requested a review from a team as a code owner June 27, 2024 11:34
@Druue Druue requested review from jkomyno and removed request for a team June 27, 2024 11:34
@Druue Druue changed the title feat(fmt): lsp hover feat(fmt): lsp hover structure Jun 27, 2024
@Druue Druue changed the title feat(fmt): lsp hover structure feat(fmt): lsp hover Jun 27, 2024
prisma-fmt/src/hover.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jkomyno jkomyno left a comment

Choose a reason for hiding this comment

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

Approved, nice HoverContext abstraction, but I'm unsure about all those info! statements

prisma-fmt/src/hover.rs Outdated Show resolved Hide resolved
prisma-fmt/src/hover.rs Outdated Show resolved Hide resolved
prisma-fmt/src/hover.rs Outdated Show resolved Hide resolved
prisma-fmt/src/hover.rs Outdated Show resolved Hide resolved
prisma-fmt/src/hover.rs Outdated Show resolved Hide resolved
@Druue Druue changed the title feat(fmt): lsp hover feat(fmt): lsp hover - model discovery Jul 3, 2024
@Druue Druue changed the title feat(fmt): lsp hover - model discovery feat(fmt): lsp hover Jul 3, 2024
Druue added a commit to prisma/language-tools that referenced this pull request Jul 4, 2024
Druue added a commit to prisma/language-tools that referenced this pull request Jul 5, 2024
Druue and others added 22 commits July 10, 2024 11:09
…r actually returned as a valid variant, so now it is.

Made `iter_tops` on walkers public. Unsure if there's a better way about this; it seemed the most convenient for right now.

Added a helper to FieldType to retrieve names.

Further clean-up of hover
use topwalker
Show field doc on hover of model and composite field names

add conversion methods to enum / composite type scalar fields for composite type field walker

general cleanup
* remove early bailout in parser db
* We can no longer assume that relation fields exist as the opposite side of the relation might not exist yet.
* Added refine_known for usage in QE; refine is now optional as we cannot guarantee a field is either a scalar or relation outside of the QE

---------

Co-authored-by: Sergey Tatarintsev <[email protected]>
Copy link
Contributor

@Weakky Weakky left a comment

Choose a reason for hiding this comment

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

Nice work, my comments are mostly questions & suggestions.

Comment on lines 28 to 36
/// ! Commenting this yields a panic
/// ! thread 'hover::tests::model_from_model_type_broken_relations' panicked at
/// ! psl/parser-database/src/walkers/relation_field.rs:120:36:
/// ! no entry found for key
// model Forum {
// id Int @id
// name String
// Post interm[]
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate as to why it panics?

Copy link
Contributor Author

@Druue Druue Jul 10, 2024

Choose a reason for hiding this comment

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

Ah good catch, this no-longer panics due to the parser database changes.

So, what used to happen was as we were building up the parser database, if we saw validation errors in the schema, we'd bailout early. The problem there was that everything using the parser database assumed that information was correct and available. This lead to indexing on things that could not exist yielding panics.

I'm going to remove the commented out model and text as this schema is now be parseable 👍

psl/parser-database/src/attributes.rs Show resolved Hide resolved
psl/parser-database/src/walkers/relation_field.rs Outdated Show resolved Hide resolved
psl/parser-database/src/walkers/relation.rs Outdated Show resolved Hide resolved
prisma-fmt/src/hover.rs Outdated Show resolved Hide resolved
},
walkers::RefinedFieldWalker::Relation(rf) => {
let opposite_model = rf.related_model();
let relation_info = rf.opposite_relation_field().map(|rf| (rf, rf.ast_field()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain when the opposite_relation_field may be None? I'm trying to understand when would format_relation_info return empty strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets say we have the following schema

model interm {
    id Int @id

    forumId Int
    forum   Forum @relation(fields: [forumId], references: [id])
}


/// Forum Doc
model Forum {
    id Int @id
}

The opposite relation is yet to be defined in the model Forum but we still want to be able to hover over where it's used as a field type in interm and see it's model documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

I see we keep c&p'ing this test runner. I'm not necessarily asking you to take action, but thinking about ways to dry this would be great. Things like take_cursor, take_cursor_one, and format_chunk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, no, this is one that I'm already thinking about 😓 I want to reduce this but didn't want to mess with it for this pr

added test for embedded m2n in mongodb

Co-authored-by: Flavian Desverne <[email protected]>
@Druue Druue merged commit 5954db1 into main Jul 10, 2024
207 checks passed
@Druue Druue deleted the feat/hover branch July 10, 2024 12:38
Druue added a commit to prisma/language-tools that referenced this pull request Jul 10, 2024
Druue added a commit to prisma/language-tools that referenced this pull request Jul 11, 2024
Druue added a commit to prisma/language-tools that referenced this pull request Jul 11, 2024
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.

Relation information tooltip
3 participants