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: refresh project on cairo_project.toml change, register text document sync on server side #6316

Conversation

piotmag769
Copy link
Collaborator

@piotmag769 piotmag769 commented Aug 29, 2024

This change is Reviewable

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 all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @Arcticae, @Draggu, and @orizi)

@piotmag769 piotmag769 changed the title LS: refresh on cairo_project.toml change LS: refresh project on cairo_project.toml change, register text document sync on server side Aug 29, 2024
Copy link
Collaborator

@Draggu Draggu left a comment

Choose a reason for hiding this comment

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

Please move this logic into client_capabilities.rs.

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @Arcticae and @orizi)

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


crates/cairo-lang-language-server/src/lib.rs line 850 at r2 (raw file):

                serde_json::to_value(TextDocumentChangeRegistrationOptions {
                    document_selector,
                    sync_kind: 2, // TextDocumentSyncKind::FULL

but FULL is 1, while INCREMENTAL is 2 according to lsp_types. isn't something off here?


crates/cairo-lang-language-server/src/lib.rs line 863 at r2 (raw file):

            for (method, options) in zip(methods, register_options) {
                register_dynamically(method, Some(options), &self.client).await;
            }

I sense overengineering here. Why not just something like this:

Suggestion:

            self.register_dynamic_capability(
                "textDocument/didOpen"
                &text_document_registration_options,
            ).await;
            self.register_dynamic_capability(
                "textDocument/didChange",
                TextDocumentChangeRegistrationOptions {
                    document_selector,
                    sync_kind: 2, // TextDocumentSyncKind::FULL
                },
            ).await;
            self.register_dynamic_capability(
                "textDocument/didSave",
                TextDocumentSaveRegistrationOptions {
                    include_text: Some(false),
                    text_document_registration_options: text_document_registration_options.clone(),
                },
            ).await;
            self.register_dynamic_capability(
                "textDocument/didClose",
                &text_document_registration_options,
            ).await;

crates/cairo-lang-language-server/src/lib.rs line 1027 at r2 (raw file):

    let result = client.register_capability(vec![registration]).await;
    if let Err(err) = result {
        warn!("Failed to register {method} event: {err:#?}");

wdyt

Suggestion:

error!("failed to register dynamic capabilities for {method}: {err:#?}");

crates/cairo-lang-language-server/src/lsp/client_capabilities.rs line 18 at r2 (raw file):

    fn workspace_configuration_support(&self) -> bool;

    /// The client supports dynamic registration for the `workspace/didChangeWatchedFiles`

fix doc

Copy link
Collaborator Author

@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 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @mkaput, and @orizi)


crates/cairo-lang-language-server/src/lib.rs line 850 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

but FULL is 1, while INCREMENTAL is 2 according to lsp_types. isn't something off here?

Done.


crates/cairo-lang-language-server/src/lib.rs line 863 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I sense overengineering here. Why not just something like this:

Done, did this before, maybe it is indeed better


crates/cairo-lang-language-server/src/lib.rs line 1027 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

wdyt

Done.


crates/cairo-lang-language-server/src/lsp/client_capabilities.rs line 18 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

fix doc

Done.

Copy link
Collaborator Author

@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 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @mkaput, and @orizi)

@piotmag769 piotmag769 force-pushed the 6236-call-detect_crate_for-whenever-project-manifest-file-is-saved branch from 505eb35 to 381d6ae Compare August 30, 2024 12:02
Copy link
Collaborator Author

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

Will do in a separate PR after we merge relevant stuff

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @mkaput, and @orizi)

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


crates/cairo-lang-language-server/src/lib.rs line 371 at r4 (raw file):

            register_options: Some(serde_json::to_value(register_options).unwrap()),
        };
        let result = self.client.register_capability(vec![registration]).await;

oh I see now that this support batch registration. could we make use of that?

@piotmag769 piotmag769 requested review from mkaput and Draggu August 30, 2024 13:46
Copy link
Collaborator Author

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

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


crates/cairo-lang-language-server/src/lib.rs line 371 at r4 (raw file):

Previously, mkaput (Marek Kaput) wrote…

oh I see now that this support batch registration. could we make use of that?

Sure, completely missed that

Copy link
Collaborator Author

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

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.

BTW I think we should do a separate server_capabilities.rs

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


crates/cairo-lang-language-server/src/lib.rs line 419 at r5 (raw file):

                    serde_json::to_value(TextDocumentChangeRegistrationOptions {
                        document_selector,
                        sync_kind: 1, // TextDocumentSyncKind::FULL

Suggestion:

sync_kind: TextDocumentSyncKind::FULL as i32,

Copy link
Collaborator Author

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

Done in slightly different way

Reviewable status: 1 of 7 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)


crates/cairo-lang-language-server/src/lib.rs line 419 at r5 (raw file):

                    serde_json::to_value(TextDocumentChangeRegistrationOptions {
                        document_selector,
                        sync_kind: 1, // TextDocumentSyncKind::FULL

Cannot be done, it is an impl const, not an enum

Copy link
Collaborator Author

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

@piotmag769 piotmag769 requested a review from mkaput September 2, 2024 12:24
@piotmag769 piotmag769 force-pushed the 6236-call-detect_crate_for-whenever-project-manifest-file-is-saved branch from d43e015 to 3737074 Compare September 2, 2024 12:28
Copy link
Collaborator

@Draggu Draggu 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 1 of 2 files at r4, 4 of 6 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @mkaput, and @orizi)

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:

Reviewed 4 of 6 files at r6, 1 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @orizi)

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 2 files at r4, 3 of 6 files at r6, 1 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae)

@piotmag769 piotmag769 added this pull request to the merge queue Sep 2, 2024
Merged via the queue into main with commit 22322f9 Sep 2, 2024
45 checks passed
@piotmag769 piotmag769 deleted the 6236-call-detect_crate_for-whenever-project-manifest-file-is-saved branch September 2, 2024 14:56
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.

4 participants