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: Add hovers for path segments #6649

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Conversation

integraledelebesgue
Copy link
Collaborator

@integraledelebesgue integraledelebesgue commented Nov 13, 2024

Closes #6540

Changes:

  • Modules appearing in paths have proper hovers containing information about their location
  • Hovers look exactly the same as in Rust Analyzer

@reviewable-StarkWare
Copy link

This change is Reviewable

@integraledelebesgue integraledelebesgue changed the title LS: Add hover for path segments LS: Add hovers for path segments Nov 13, 2024
@integraledelebesgue integraledelebesgue force-pushed the fix/path-segment-hover branch 2 times, most recently from a05af38 to 10198aa Compare November 13, 2024 12:52
@integraledelebesgue integraledelebesgue marked this pull request as ready for review November 13, 2024 12:53
Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @integraledelebesgue, @mkaput, and @orizi)


crates/cairo-lang-language-server/src/lang/inspect/defs.rs line 330 at r1 (raw file):

    /// Gets the module's documentation if it's available.
    pub fn documentation(&self) -> Option<String> {
        // Modules cannot have docstrings at the moment.

This is not true, get_item_documentation should deal with it

Copy link
Member

@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.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @integraledelebesgue, and @orizi)


crates/cairo-lang-language-server/src/lang/inspect/defs.rs line 313 at r1 (raw file):

                Some(stripped_path) => stripped_path.to_owned(),
                None => full_path,
            };

this is parent module full path. term full path as used here is very misleading

Code quote:

        let full_path =
            match full_path.strip_suffix(name.as_str()).and_then(|path| path.strip_suffix("::")) {
                Some(stripped_path) => stripped_path.to_owned(),
                None => full_path,
            };

@integraledelebesgue integraledelebesgue force-pushed the fix/path-segment-hover branch 2 times, most recently from 55e018f to e22ffcb Compare November 15, 2024 12:00
Copy link
Collaborator Author

@integraledelebesgue integraledelebesgue 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: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, @orizi, and @piotmag769)


crates/cairo-lang-language-server/src/lang/inspect/defs.rs line 313 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

this is parent module full path. term full path as used here is very misleading

I rebuilt the path logic a bit and also changed those confusing names


crates/cairo-lang-language-server/src/lang/inspect/defs.rs line 330 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This is not true, get_item_documentation should deal with it

You're right. I added a proper decumentation retrieval logic

Copy link
Member

@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.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

Copy link
Member

@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.

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

Copy link
Collaborator

@piotmag769 piotmag769 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 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @integraledelebesgue, and @orizi)


crates/cairo-lang-language-server/src/lang/inspect/defs.rs line 101 at r2 (raw file):

            }

            ResolvedItem::Generic(ResolvedGenericItem::Module(id)) => {

Out of curiosity: when do we enter each branch? What is a generic module? ;p

Copy link
Collaborator Author

@integraledelebesgue integraledelebesgue 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: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)


crates/cairo-lang-language-server/src/lang/inspect/defs.rs line 101 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Out of curiosity: when do we enter each branch? What is a generic module? ;p

I have no idea which semantic construct it corresponds to tbh. I just separated all branches somehow related to modules :)

Copy link
Member

@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: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

@integraledelebesgue integraledelebesgue added this pull request to the merge queue Nov 18, 2024
@integraledelebesgue integraledelebesgue removed this pull request from the merge queue due to a manual request Nov 18, 2024
@integraledelebesgue integraledelebesgue added this pull request to the merge queue Nov 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to Branch Protection failures Nov 18, 2024
You're not authorized to push to this branch. Visit "About protected branches" for more information.
@integraledelebesgue integraledelebesgue added this pull request to the merge queue Nov 18, 2024
Merged via the queue into main with commit 9e068e5 Nov 18, 2024
47 of 48 checks passed
@integraledelebesgue integraledelebesgue deleted the fix/path-segment-hover branch November 18, 2024 11:15
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.

LS: Incorrect hovers for path segments
4 participants