Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_lsp): rename off by default for vscode #3473

Merged
merged 6 commits into from
Oct 25, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Oct 21, 2022

Summary

This PR sets renaming as off by default.

I have not found a way to change Server capability inside did_configuration_changed, so I decided to register the LSP server capable of renaming, but forfeiting the request when the config is null or false.

I am also notifying the user when vscode starts and the debug environment variable is being used.

Test Plan

on Windows

> set-item env:/DEBUG_SERVER_PATH "C:\github\tools\target\debug\rome.exe";
> ps rome | stop-process
> cargo build -p rome_cli-dev

Open a JS/TS file. We will see:

[Trace - 07:20:36] Sending response 'workspace/configuration - (0)'. Processing request took 0ms
Result: [
    {
        "lspBin": "C:/github/tools/target/debug/rome.exe",
        "rename": null
    }
]

Which means the rename config is defaulting to null yet.
Later we see:

[Trace - 07:20:36] Received request 'client/unregisterCapability - (2)'.
Params: {
    "unregisterations": [
        {
            "id": "textDocument/rename",
            "method": "textDocument/rename"
        }
    ]
}


[Trace - 07:20:36] Sending response 'client/unregisterCapability - (2)'. Processing request took 1ms
No result returned.

If we rename a variable now, no rename request will be seen.
If we change the variable to true. We will see :

[Trace - 07:22:47] Received request 'client/registerCapability - (5)'.
Params: {
    "registrations": [
        {
            "id": "textDocument/rename",
            "method": "textDocument/rename"
        }
    ]
}


[Trace - 07:22:47] Sending response 'client/registerCapability - (5)'. Processing request took 0ms
No result returned.

Now if we rename a variable we see the rename request

[Trace - 07:23:34] Sending request 'textDocument/rename - (11)'.
Params: {
    "textDocument": {
        "uri": "file:///c%3A/github/tools/website/utils.js"
    },
    "position": {
        "line": 1,
        "character": 8
    },
    "newName": "ext2"
}

@xunilrj xunilrj requested a review from ematipico as a code owner October 21, 2022 18:50
@xunilrj xunilrj requested a review from a team October 21, 2022 18:50
@netlify
Copy link

netlify bot commented Oct 21, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 5dd3c41
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6357bb3a582baa00078a0ec8

@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 18:50 Inactive
@@ -23,7 +23,7 @@ rome_flags = { path = "../rome_flags" }
rome_rowan = { path = "../rome_rowan" }
rome_console = { path = "../rome_console" }
rome_text_edit = { path = "../rome_text_edit" }
tokio = { workspace = true, features = ["io-std"] }
tokio = { workspace = true, features = ["rt", "io-std"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rome_lsp was not compiling

@@ -167,6 +167,30 @@ impl LanguageServer for LSPServer {
Ok(())
}

#[tracing::instrument(level = "trace", skip(self))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just sorted these fns.

@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 18:52 Inactive
@github-actions
Copy link

github-actions bot commented Oct 21, 2022

@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 19:00 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 22:48 Inactive
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Could you please do some testing on the extension itself? By looking at your test plan, it seems that hasn't bee done

@xunilrj xunilrj temporarily deployed to netlify-playground October 25, 2022 10:26 Inactive
@xunilrj
Copy link
Contributor Author

xunilrj commented Oct 25, 2022

Could you please do some testing on the extension itself? By looking at your test plan, it seems that hasn't bee done

Done.

@xunilrj xunilrj temporarily deployed to netlify-playground October 25, 2022 10:32 Inactive
@xunilrj xunilrj merged commit cc08105 into main Oct 25, 2022
@xunilrj xunilrj deleted the fix/rename-off-by-default branch October 25, 2022 11:16
@ematipico ematipico added this to the 10.0.0 milestone Oct 28, 2022
@ematipico ematipico added E-VScode Editors: VSCode A-Editors Area: editors labels Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Editors Area: editors E-VScode Editors: VSCode
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants