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

LS: Clean up hover definition logic #5780

Merged
merged 1 commit into from
Jun 13, 2024
Merged

LS: Clean up hover definition logic #5780

merged 1 commit into from
Jun 13, 2024

Conversation

mkaput
Copy link
Member

@mkaput mkaput commented Jun 11, 2024

  1. Reorganized logic scaffolding to resemble RA's code.
  2. Added Markdown utility and added unit tests.
  3. Moved ///
    stripping into Salsa as this looks like a more proper place.
  4. Converted everything to use MarkupContent,
    as using MarkedString is deprecated in protocol.
  5. Added highlights for hover targets,
    now identifiers get nice background in VSCode when hovering over them.

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 11 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mkaput)


crates/cairo-lang-defs/src/db.rs line 113 at r2 (raw file):

    // TODO(mkaput): Add tests.
    // TODO(mkaput): Support #[doc] attribute. This will be a bigger chunk of work because it would
    //   be the best to convert all /// comments to #[doc] attrs before processing items by plugins,

not sure if these should be equivalent exactly - but indeed should be easily accessible.

Copy link
Member Author

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mkaput)


crates/cairo-lang-defs/src/db.rs line 113 at r2 (raw file):

Previously, orizi wrote…

not sure if these should be equivalent exactly - but indeed should be easily accessible.

In rust they are all are concatenated. You can mix ///, #[doc], //! and #![doc] on the same item and all are concatenated in source code order. Concatenation happens before macros, so macro_rules! are only seeing one #[doc] attribute when matching for example.

@mkaput mkaput force-pushed the spr/main/56d17f49 branch from ed41a02 to eda9018 Compare June 13, 2024 07:47
@mkaput mkaput force-pushed the spr/main/66d7352c branch from 84a3798 to 1dc5e48 Compare June 13, 2024 07:47
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mkaput)

1. Reorganized logic scaffolding to resemble RA's code.
2. Added `Markdown` utility and added unit tests.
3. Moved `///`
stripping into Salsa as this looks like a more proper place.
4. Converted everything to use `MarkupContent`,
as using `MarkedString` is deprecated in protocol.
5. Added highlights for hover targets,
now identifiers get nice background in VSCode when hovering over them.

commit-id:56d17f49
@mkaput mkaput changed the base branch from spr/main/66d7352c to main June 13, 2024 11:59
@mkaput mkaput force-pushed the spr/main/56d17f49 branch from eda9018 to 827cfaa Compare June 13, 2024 11:59
@mkaput mkaput enabled auto-merge June 13, 2024 12:03
@mkaput mkaput added this pull request to the merge queue Jun 13, 2024
Merged via the queue into main with commit db405e9 Jun 13, 2024
43 checks passed
@mkaput mkaput deleted the spr/main/56d17f49 branch June 13, 2024 12:32
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.

2 participants