Closed beta-ziliani closed 2 years ago
Hello @beta-ziliani, was wondering why not simply inherit error too? That way, you only need to check for success of process status and raise the single error (shown at the bottom) after all the error has been automatically output to you, no need to check if there were errors collected (IO::Memory
) and then send that to STDERR later.
I think would be simpler code, but maybe I'm missing what you're trying to achieve as DX.
Cheers.
Currently, in case of failure the captured STDERR is part of the error message.
If we just pass STDERR through, the output from crystal build
would be printed independent of the error and not be part of the error message.
I think it's better to keep it as part of the error message for consistency. That requires capturing and reprinting, but it's not a big deal.
So in the case of warning:
❯ shards build
Dependencies are satisfied
Building: d
In d.cr:9:1
9 | a
^
Warning: Deprecated top-level a.
A total of 1 warnings were found.
In the case of errors:
❯ shards build
Dependencies are satisfied
Building: d
Error target d failed to compile:
Showing last frame. Use --error-trace for full trace.
In d.cr:3:3
3 | foo
^--
Error: undefined local variable or method 'foo' for top-level
Thought since error is being displayed multiple times, the message about the failed target got lost in the middle, instead of something like:
❯ shards build
Dependencies are satisfied
Building: d
Showing last frame. Use --error-trace for full trace.
In d.cr:3:3
3 | foo
^--
Error: undefined local variable or method 'foo' for top-level
Target d failed to compile
But what you propose makes sense, to make a section in the output with the error. 👍🏽
Cheers.
We could certainly change the order in the error message! That's not the point.
I'm concerned about maintaining that the message of the exception contains all the error information.
I'm concerned about maintaining that the message of the exception contains all the error information.
Not sure I follow your concern on that, from what I understand is that Crystal outputs the error message entirely to STDERR, so the error is captured with the current approach.
$ crystal build d.cr --error-trace
In d.cr:9:1
9 | a
^
Warning: Deprecated top-level a.
A total of 1 warnings were found.
$ crystal build d.cr --error-trace 2>/dev/null
$ echo $?
0
Same for errors:
$ crystal build d.cr --error-trace
In d.cr:10:1
10 | foo
^--
Error: undefined local variable or method 'foo' for top-level
$ crystal build d.cr --error-trace 2>/dev/null
$ echo $?
1
Thank you.
@luislavena I meant that the error output from crystal build
is contained in the Error
exception.
Solves #535 by outputting the STDERR of the
crystal
build to the STDERR of theshards
process only if the build is successful. This allows to notice deprecation warnings and the like.In order to test it, I had to concatenate the STDOUT and STDERR, which isn't as clean as possible but works.