-
Notifications
You must be signed in to change notification settings - Fork 62
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
Cabal cradle: change error message on failure #353
Conversation
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.
LGTM, one small detail up for disucssion, imo
src/HIE/Bios/Cradle.hs
Outdated
when (ex /= ExitSuccess) $ do | ||
deps <- liftIO $ cabalCradleDependencies workDir workDir | ||
let cmd = show (["cabal", cabalCommand] <> cabalArgs) | ||
let errorMsg = "Failed to run " <> cmd <> " in directory \"" <> workDir <> "\". See below for full command and error." |
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.
I am not sure that "See below for full command and error." is going to be much helpful to users, since where is "below"?
Not sure this sentence is necessary.
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.
Maybe "See stderr for full command and error details"?. I think it's helpful to point out that there's more information available
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.
I agree, but if this is shown in the LSP dialog pop-up, "below" has no meaning. However, I was actually unable to provoke that pop-up window, is this only shown as a diagnostic?
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.
I think you would need to build HLS with a hie-bios
that includes #347 to get errors automatically shown to the user. I do agree that "below" is ambiguous here. Maybe "consult the logs"?
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.
@fendor Yes, it's only a diagnostic (on the first line of the module). @michaelpj 👍 I changed the error msg like you suggested
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.
LGTM, thanks!
This changes the first line of the error message that is shown when the cabal cradle fails to
(the error message was
Failed to parse result of calling cabal
before, which is less informative).cc @fendor