Propagate build errors using window/showMessage #657
Conversation
|
To clarify, this is meant to only use |
|
Yes. When cargo bombs out. |
|
Technically should also do it when rustc does for plan.rs, but there doesn't seem to be an error case there right now. Maybe it just always works. |
algesten
force-pushed
the
build-err
branch
4 times, most recently
from
3971938 to
abd4727
Compare
|
@nrc This PR was made redundant by the work you did a few days ago. I still think there's a point to keep the error messages concise against VSCode (because it will only show one single line), so I redid this PR to only do some of that minor formatting. |
src/actions/progress.rs
Outdated
| fn notify_end_diagnostics(&self); | ||
| fn notify_error_diagnostics(&self, msg: &str); |
There was a problem hiding this comment.
I don't think this rename is necessary
There was a problem hiding this comment.
ok. i keep the name
src/build/cargo.rs
Outdated
| }; | ||
| let msg = format!("Cargo failed: {}{}", err, stdout_msg); | ||
| info!("{}", msg); | ||
| BuildResult::Err(msg, None) |
There was a problem hiding this comment.
This message is longer than previously, so I don't think it makes it more concise.
There was a problem hiding this comment.
Really? I mean I tested this live (see screen grab at the top).
The current code would show:
cargo failed
cause: Could not compile `getopts`.
stdout:
Which has two problems:
- VSCode only shows one line. So the user would only see
cargo failed, not the cause. - It prints
stdout:regardless of there being anything to show there.
The proposed change would do
Cargo failed: Could not compile `getopts`.
And if there is a stdout pad that on inside parentheses. I maintain this is a better solution.
src/build/cargo.rs
Outdated
| format!("({})", stdout) | ||
| }; | ||
| let msg = format!("Cargo failed: {}{}", err, stdout_msg); | ||
| info!("{}", msg); |
There was a problem hiding this comment.
I try to use debug for messages where something has gone wrong.
src/actions/post_build.rs
Outdated
| // It's not a good idea to make a long message here, the output in | ||
| // VSCode is one single line, and it's important to capture the | ||
| // root cause. | ||
| // let msg = format!("There was an error trying to build, RLS features will be limited: {}", cause); |
There was a problem hiding this comment.
Just delete rather than commenting out.
src/actions/post_build.rs
Outdated
| // VSCode is one single line, and it's important to capture the | ||
| // root cause. | ||
| // let msg = format!("There was an error trying to build, RLS features will be limited: {}", cause); | ||
| self.notifier.notify_build_error_diagnostics(cause); |
There was a problem hiding this comment.
It sounds like a good idea to make the message shorter, but we probably need some boilerplate here - the cause message might not be very informational if it is an internal error message and we should make clear to the user that something failed.
There was a problem hiding this comment.
Yes. It's a hard balance. I think Cargo failed: that comes from the underlying error addresses that somewhat. I don't know if there is some other mechanism we can use to provide more details to the user?
|
@nrc I pushed the changes I agreed with and discussed the rest above. |
|
Thanks for the changes! |
This is based off #653 and needs to land after that.
Currently build errors go to the logs, but does not show up in the UI. This PR fixes that so that the first encountered build error will be notified using
window/showMessage. No further build errors will be propagated until the error state has cleared since a build attempt happens on every keystroke.