Add support for haskell-debugger#33
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Rajkumar Natarajan.
|
c4bf776 to
a7133e9
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have @rajcspsg on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
MrSubidubi
left a comment
There was a problem hiding this comment.
Thanks for this!
Preliminary review, will do my best to get back to this in a timely manner again
There was a problem hiding this comment.
Let's call this language_server.rs instead
There was a problem hiding this comment.
Just wondering why this change: haskell.rs -> language_server.rs is needed.
It might be fine from Rust perspective, but by following folder structures of other Zed extension, especially with debuggers included), most extensions name its main entry file as same as the language.
Just to mention few (randomly - all with debugger support):
| name = "Haskell" | ||
| description = "Haskell support." | ||
| version = "0.2.1" | ||
| version = "0.2.2" |
There was a problem hiding this comment.
Ci will lint this, but that will be done after automatically (and will be minor at least)
Co-authored-by: Finn Evers <finn.evers@outlook.de>
|
@MrSubidubi incorporated your review comments |
|
Will try to get back to you with a final review this week, apologies for the delay! |
sectore
left a comment
There was a problem hiding this comment.
Great addition. Just tested it on my local machine and it works great.
Tested manually with:
ghc --version
The Glorious Glasgow Haskell Compilation System, version 9.14.1
hdb --version
Haskell Debugger, version 0.13.1.0
zeditor --version
Zed 1.6.3Just few nits have been added from my side.
| format!("Invalid Haskell debug configuration (expected JSON object): {err}") | ||
| })?; | ||
| map.entry("type".to_string()) | ||
| .or_insert_with(|| json!(HASKELL_DEBUG_ADAPTER)); |
There was a problem hiding this comment.
(nit) or_insert_with -> or_insert
.or_insert(json!(HASKELL_DEBUG_ADAPTER));| map.entry("type".to_string()) | ||
| .or_insert_with(|| json!(HASKELL_DEBUG_ADAPTER)); | ||
| map.entry("request".to_string()) | ||
| .or_insert_with(|| json!("launch")); |
There was a problem hiding this comment.
(nit) or_insert_with -> or_insert
.or_insert(json!("launch"));| map.entry("request".to_string()) | ||
| .or_insert_with(|| json!("launch")); | ||
| map.entry("name".to_string()) | ||
| .or_insert_with(|| json!(task.label.clone())); |
There was a problem hiding this comment.
(nit) or_insert_with -> or_insert
.or_insert(json!(task.label.clone()));| fn path_components_to_string(path: &Path) -> Result<String, String> { | ||
| let mut out = String::new(); | ||
| for (i, component) in path.components().enumerate() { | ||
| match component { | ||
| Component::Normal(part) => { | ||
| if i > 0 { | ||
| out.push(std::path::MAIN_SEPARATOR); | ||
| } | ||
| out.push_str(&part.to_string_lossy()); | ||
| } | ||
| Component::CurDir => {} | ||
| Component::ParentDir => { | ||
| return Err("Program path must not contain `..`.".into()); | ||
| } | ||
| Component::RootDir | Component::Prefix(_) => { | ||
| return Err("Invalid program path.".into()); | ||
| } | ||
| } | ||
| } | ||
| if out.is_empty() { | ||
| Err("Resolved entry file path is empty.".into()) | ||
| } else { | ||
| Ok(out) | ||
| } | ||
| } |
There was a problem hiding this comment.
(nit) path_components_to_string function can be more compact. No mutation of out: String, no loop, no if i > 0 check needed anymore.
fn path_components_to_string(path: &Path) -> Result<String, String> {
path.components()
.filter_map(|c| match c {
Component::Normal(part) => Some(Ok(part.to_string_lossy().into_owned())),
Component::CurDir => None,
Component::ParentDir => Some(Err("Program path must not contain `..`.".into())),
_ => Some(Err("Invalid program path.".into())),
})
.collect::<Result<Vec<_>, _>>()
.and_then(|parts| {
if parts.is_empty() {
Err("Resolved entry file path is empty.".into())
} else {
Ok(parts.join(std::path::MAIN_SEPARATOR_STR))
}
})
}| adapter_name: String, | ||
| config: zed::serde_json::Value, | ||
| ) -> Result<StartDebuggingRequestArgumentsRequest, String> { | ||
| crate::debugger::dap_request_kind(adapter_name.as_str(), &config) |
There was a problem hiding this comment.
(nit-nit) &adapter_name if you want save as_str(). Just in case :)
There was a problem hiding this comment.
Just wondering why this change: haskell.rs -> language_server.rs is needed.
It might be fine from Rust perspective, but by following folder structures of other Zed extension, especially with debuggers included), most extensions name its main entry file as same as the language.
Just to mention few (randomly - all with debugger support):
|
|
||
| [lib] | ||
| path = "src/haskell.rs" | ||
| path = "src/lib.rs" |
There was a problem hiding this comment.
| } | ||
| out.push_str(&part.to_string_lossy()); | ||
| } | ||
| Component::CurDir => {} |
There was a problem hiding this comment.
By enumerating and counting i, CurDir of ./src/Main.hs will become /src/Main.hs. Even by skipping CurDir by {}, next count of i runs Component::Normal again and adds the leading /.
That's a bug and can be fixed with a suggested change in this comment
@MrSubidubi Any chance for another review? Pls consider latest review comments. cc/ @rajcspsg |
closes #12
screen recording for debugger support