On macro run error: show status, stdout and stderr #4614

bew · 2017-06-23T17:52:58Z

First PR to display more information about a failing macro run.
Another PR might come to display the errors without the backtrace from ecr/processor to properly resolve #4577

Given thoses files:

# processor.cr

def func
  raise "Some exception happened" # This will always fail
end

puts %(puts "some stuff on stdout")

begin
  func
rescue e
  STDERR.puts "ERROR: #{e}"
  exit 1
end

# test_run.cr

{{ run "./processor" }}

Compilation will give:

$ crystal ./test_run.cr
Error in test_run.cr:1: expanding macro

{{ run "./processor" }}
^

in test_run.cr:1: Error executing run (exit code: 1): ./processor 

---- Got on stdout: ----

puts "some stuff on stdout"


---- Got on stderr: ----

ERROR: Some exception happened



{{ run "./processor" }}
   ^~~

If you have any other ideas how to present stdout & stderr, I'm open to suggestion.

There is currently no specs on the macro method run, should I try to make one for this?

RX14 · 2017-07-06T16:20:30Z

Somehow I don't like the "---- Got on stdout: ----" lines, they seem to have a different look and feel to the rest of the compiler error output. I get that that's a wishy-washy complaint but it feels wrong.

bew · 2017-07-06T16:59:42Z

@RX14 I agree completely, but I don't have any idea how this could be presented.
If you have one, I'll be happy to do it.

straight-shoota · 2017-07-06T20:52:29Z

Maybe indent the stream output?

in test_run.cr:1: Error executing run (exit code: 1): ./processor 

    puts "some stuff on stdout"

stderr:

    ERROR: Some exception happened

    {{ run "./processor" }}
       ^~~

ysbaddaden

Maybe indent. Otherwise 👌

bew · 2017-07-07T08:32:28Z

Thanks a lot for your approvals ;)
@straight-shoota I'll do the indent, it's wayy nicer, thanks!

bew · 2017-07-07T08:55:29Z

Following @straight-shoota suggestion, it now looks like:

Error in test_run.cr:1: expanding macro

{{ run "./processor" }}
^

in test_run.cr:1: Error executing run (exit code: 1): ./processor 

stdout:

    puts "some stuff on stdout"

stderr:

    ERROR: Some exception happened


{{ run "./processor" }}
   ^~~

bew · 2017-07-07T09:03:39Z

Also, the error message is in bold, do I try to remove it for stdout and stderr?

RX14 · 2017-07-07T10:08:05Z

@bew stdout: and stderr: should be bold but the output shouldn't be.

bew · 2017-07-07T11:39:02Z

stdout: and stderr: should be bold but the output shouldn't be.

@RX14 How should I go about this? I couldn't get colorize to work to remove any coloring/format for a specific text.

RX14 · 2017-08-17T09:16:32Z

@bew In this case you'll probably have to manually insert the "reset" escape code ("\e[0") to reset the formatting, then use colorize after that.

bew · 2017-08-17T10:01:00Z

Rebased. Thanks @RX14 for the tip. Even if I don't like hardcoding the terminal escape code, and kind of bypassing the Colorize module, it has the merit to work!

ysbaddaden · 2017-08-18T11:23:10Z

src/compiler/crystal/macros/methods.cr

+        if result.stdout.empty? && result.stderr.empty?
+          message << "\nGot no output."
+        else
+          message << "\e[0m" if Colorize.enabled?


Maybe a Colorize#reset method would be interesting?

It would do it, yes, should I open another PR for this?

Would you mind squashing your commits then add another one that implements Colorize#reset?

@ysbaddaden how would it be used?

message << Colorize.reset message << "".colorize.reset # --> does it make sense to have: "abc".colorize.reset ? Colorize.reset(message)

Yes, I believe "abc".colorize.reset makes sense: we want to print a string and make sure the ASCII escape codes have been reset.

Thought this is about disabling escape codes, it's still adding escape codes to the string, so we should follow the actual API (as .bold, ...)

for "abc".colorize.reset, should I save the current formatting, reset, put "abc", then restore the formatting? Or should I go simple for this PR, and just reset and put the text?

I actually think that Colorize.reset is a better API. Using String#colorize implies you want to do something to a string, and most importantly that whatever changes you made will be reset afterwards. With String#colorize.reset you don't get that, there's no way to restore the old colours at the end of the string, so I don't think that API makes sense.

bew · 2017-08-18T13:56:59Z

So I rebased, squashed, and added Colorize.reset(io).
Good to go? 👍

RX14 · 2017-08-18T14:05:31Z

Perhaps the Colorize.reset commit should come before the other one so it can use it directly?

bew · 2017-08-18T14:10:47Z

@RX14 done.

bew

Just discovered I missed something, what do I do? new PR with the fix?

bew · 2017-08-18T18:20:31Z

src/compiler/crystal/macros/methods.cr

+            message.puts
+            result.stderr.each_line do |line|
+              message << "    "
+              message << line


I missed the \n

bew · 2017-08-18T18:20:44Z

src/compiler/crystal/macros/methods.cr

+            message.puts
+            result.stdout.each_line do |line|
+              message << "    "
+              message << line


I missed the \n

bew force-pushed the show-errors-for-macro-run branch from b21d840 to 79b06e3 Compare June 27, 2017 16:04

asterite approved these changes Jul 6, 2017

View reviewed changes

ysbaddaden approved these changes Jul 7, 2017

View reviewed changes

bew force-pushed the show-errors-for-macro-run branch 2 times, most recently from 01beb9b to 70b21d2 Compare July 7, 2017 08:55

RX14 mentioned this pull request Aug 17, 2017

Can't build Crystal after building Crystal #2195

Closed

bew force-pushed the show-errors-for-macro-run branch from 70b21d2 to 13a94f5 Compare August 17, 2017 09:57

RX14 approved these changes Aug 18, 2017

View reviewed changes

RX14 added kind:feature topic:stdlib:macros labels Aug 18, 2017

ysbaddaden reviewed Aug 18, 2017

View reviewed changes

bew force-pushed the show-errors-for-macro-run branch from 13a94f5 to 500e9fc Compare August 18, 2017 13:53

asterite approved these changes Aug 18, 2017

View reviewed changes

bew added 2 commits August 18, 2017 16:07

Add Colorize.reset

b31775a

On macro run error, show status, stdout and stderr

a7776b9

bew force-pushed the show-errors-for-macro-run branch from 500e9fc to a7776b9 Compare August 18, 2017 14:10

RX14 approved these changes Aug 18, 2017

View reviewed changes

asterite approved these changes Aug 18, 2017

View reviewed changes

RX14 merged commit 5accf4e into crystal-lang:master Aug 18, 2017

RX14 added this to the Next milestone Aug 18, 2017

bew deleted the show-errors-for-macro-run branch August 18, 2017 18:12

bew commented Aug 18, 2017

View reviewed changes

bew mentioned this pull request Aug 18, 2017

Fix macro run error for multi-line outputs #4850

Merged

straight-shoota mentioned this pull request Oct 5, 2017

Raise error for invalid paths schovi/baked_file_system#11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On macro run error: show status, stdout and stderr #4614

On macro run error: show status, stdout and stderr #4614

bew commented Jun 23, 2017 •

edited

RX14 commented Jul 6, 2017

bew commented Jul 6, 2017 •

edited

straight-shoota commented Jul 6, 2017

ysbaddaden left a comment •

edited

bew commented Jul 7, 2017

bew commented Jul 7, 2017 •

edited

bew commented Jul 7, 2017 •

edited

RX14 commented Jul 7, 2017

bew commented Jul 7, 2017 •

edited

RX14 commented Aug 17, 2017

bew commented Aug 17, 2017

ysbaddaden Aug 18, 2017

bew Aug 18, 2017

ysbaddaden Aug 18, 2017

bew Aug 18, 2017 •

edited

ysbaddaden Aug 18, 2017

bew Aug 18, 2017

RX14 Aug 18, 2017

ysbaddaden Aug 18, 2017

bew commented Aug 18, 2017 •

edited

RX14 commented Aug 18, 2017

bew commented Aug 18, 2017

bew left a comment •

edited

bew Aug 18, 2017

bew Aug 18, 2017

On macro run error: show status, stdout and stderr #4614

On macro run error: show status, stdout and stderr #4614

Conversation

bew commented Jun 23, 2017 • edited

RX14 commented Jul 6, 2017

bew commented Jul 6, 2017 • edited

straight-shoota commented Jul 6, 2017

ysbaddaden left a comment • edited

Choose a reason for hiding this comment

bew commented Jul 7, 2017

bew commented Jul 7, 2017 • edited

bew commented Jul 7, 2017 • edited

RX14 commented Jul 7, 2017

bew commented Jul 7, 2017 • edited

RX14 commented Aug 17, 2017

bew commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew commented Aug 18, 2017 • edited

RX14 commented Aug 18, 2017

bew commented Aug 18, 2017

bew left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew commented Jun 23, 2017 •

edited

bew commented Jul 6, 2017 •

edited

ysbaddaden left a comment •

edited

bew commented Jul 7, 2017 •

edited

bew commented Jul 7, 2017 •

edited

bew commented Jul 7, 2017 •

edited

bew Aug 18, 2017 •

edited

bew commented Aug 18, 2017 •

edited

bew left a comment •

edited