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: Explicitly ignore Cancel & SetTrace without polluting logs #6518

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

mkaput
Copy link
Member

@mkaput mkaput commented Oct 24, 2024

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@mkaput mkaput force-pushed the spr/main/09ef1ee1 branch from 99ad251 to 0278974 Compare October 24, 2024 15:38
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 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae, @Draggu, and @orizi)

@mkaput mkaput force-pushed the spr/main/09ef1ee1 branch from 0278974 to c39c30d Compare October 25, 2024 07:48
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.

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


crates/cairo-lang-language-server/src/server/routing/mod.rs line 97 at r1 (raw file):

        DidSaveTextDocument::METHOD => local_notification_task::<DidSaveTextDocument>(notification),

        // Ignore these notifications as they are meaningless for CairoLS.

Can we explain here that LS know when to cancel on its own, without being notified by the client? Additionally, we ignore tracing settings because increasing it will quickly lead to overwhelmingly large traces. This would be more useful than only stating that these are meaningless.


crates/cairo-lang-language-server/src/server/routing/mod.rs line 99 at r1 (raw file):

        method => {
            warn!("received notification {method} which does not have a handler");

Please keep these lines


crates/cairo-lang-language-server/src/server/routing/mod.rs line 105 at r1 (raw file):

    .unwrap_or_else(|error| {
        error!("encountered error when routing notification: {error}");

Same here

@mkaput mkaput force-pushed the spr/main/09ef1ee1 branch from c39c30d to 3b7f659 Compare October 25, 2024 10:33
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: 1 of 2 files reviewed, all discussions resolved (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)


crates/cairo-lang-language-server/src/server/routing/mod.rs line 97 at r1 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Can we explain here that LS know when to cancel on its own, without being notified by the client? Additionally, we ignore tracing settings because increasing it will quickly lead to overwhelmingly large traces. This would be more useful than only stating that these are meaningless.

Done

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:

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @Arcticae, @orizi, and @piotmag769)

Copy link
Collaborator

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

@mkaput mkaput force-pushed the spr/main/09129f74 branch from 7a7602f to 5c71563 Compare October 25, 2024 10:59
@mkaput mkaput force-pushed the spr/main/09ef1ee1 branch 3 times, most recently from fc01490 to 515c680 Compare October 25, 2024 12:00
Base automatically changed from spr/main/09129f74 to main October 25, 2024 12:26
@mkaput mkaput force-pushed the spr/main/09ef1ee1 branch from 515c680 to d2826dc Compare October 25, 2024 12:28
@mkaput mkaput enabled auto-merge October 25, 2024 12:29
@mkaput mkaput added this pull request to the merge queue Oct 25, 2024
Merged via the queue into main with commit f98c2b0 Oct 25, 2024
43 of 44 checks passed
@mkaput mkaput deleted the spr/main/09ef1ee1 branch October 25, 2024 12:51
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.

5 participants