-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add "Open Cargo.toml" action #6519
Conversation
crates/rust-analyzer/src/handlers.rs
Outdated
if !cargo_toml_path.exists() { | ||
return Ok(None); | ||
} | ||
let cargo_toml_url = to_proto::url_from_abs_path(&cargo_toml_path.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let cargo_toml_url = to_proto::url_from_abs_path(&cargo_toml_path.clone()); | |
let cargo_toml_url = to_proto::url_from_abs_path(&cargo_toml_path); |
crates/rust-analyzer/src/handlers.rs
Outdated
} | ||
let cargo_toml_url = to_proto::url_from_abs_path(&cargo_toml_path.clone()); | ||
let cargo_toml_location = | ||
Location::new(cargo_toml_url.clone(), Range::new(Position::new(0, 0), Position::new(0, 0))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Location::new(cargo_toml_url.clone(), Range::new(Position::new(0, 0), Position::new(0, 0))); | |
Location::new(cargo_toml_url, Range::default(); |
(not sure about the Default
impl)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, is the lsp_types::GotoDefinitionResponse
the right type to return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lnicola what else should I return? GotoDefinitionResponse seems to be used to jump to a specific part of a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my second time contributing to Rust-Analyzer so it might take me a bit to learn the wheels :|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. I was thinking that a custom type or an URL would be better, since we're navigating to a file, not a location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could maybe use TextDocumentIdentifier and hardcode the location on the client? But maybe in the future we want to open specific parts of the Cargo.toml, and we should keep the client logic as simple as possible, right?
697f9c7
to
6b950d2
Compare
The changes look good to me, but there's still a bit of a mess in history. Lets try to clean up. @p3achyjr I think the following steps should help:
|
e1c953c
to
b1b7727
Compare
Thanks @matklad. I was running into issues where whenever I ran The steps worked though! Will keep this in my back pocket :) |
Hm, there's still something wrong with history (.DS_Store file is still there), but I'll fix that up locally! bors r+ Thanks! |
What is it?
This adds an "open cargo.toml" action from the vscode shell, resolves #6462
Test
Ran
cargo xtask install --server
andcargo xtask install --client
, thenDeveloper: Reload Window
.When clicked: