Format: show backtrace when formatting bug found #4631
Conversation
|
It's very useful to maintain autogenerated code. LGTM. |
makenowjust
force-pushed
the
fix/crystal-format/show-backtrace-when-bug-found
branch
from
27bf6bc
to
bf45582
Compare
| STDERR.puts | ||
|
|
||
| STDERR.puts ex | ||
| ex.backtrace.each do |frame| |
There was a problem hiding this comment.
I think you shoud use ex.inspect_with_backtrace(STDERR) instead
There was a problem hiding this comment.
true, thanks I'll change it 💤
|
ping? I really want to merge it. |
| @@ -68,7 +68,7 @@ class Crystal::Command | |||
| when .invalid_byte_sequence? | |||
| error "'#{result.filename}' is not a valid Crystal source file", exit_code: nil | |||
| when .bug? | |||
| error "there's a bug formatting '#{result.filename}', please report it including the contents of the file: https://github.com/crystal-lang/crystal/issues", exit_code: nil | |||
| error "there's a bug formatting '#{result.filename}', to show more information, please run:\n\n $ crystal tool format '#{result.filename}'", exit_code: nil | |||
There was a problem hiding this comment.
Why not show the backtrace for the file here? Why do you tell the user to run format on the individual file?
There was a problem hiding this comment.
FormatResultdoes not keep an error object currently. I prefer small fix.- It is hard to read multiple backtrace, and each of errors may be not related to others. I expect to report formatter error with this backtrace.
There was a problem hiding this comment.
For the multiple-backtrace situation i would expect the formatter to report the first error the same as the single-file mode and then exit. I wonder what the rest of the team think about this but I wouldn't be opposed to storing the error.
There was a problem hiding this comment.
For the multiple-backtrace situation i would expect the formatter to report the first error the same as the single-file mode and then exit.
This behavior has a big problem: it makes hard to format all files when formatter bug is found. It should continue to format file as possible.
There was a problem hiding this comment.
Why not just print all the backtraces then? Just a simple
Error: couldn't format foo.cr, please report a bug including the contents of it
<inspect_with_backtrace>
Error: couldn't format bar.cr, please report a bug including the contents of it
<inspect_with_backtrace>
There was a problem hiding this comment.
I explained this already: "It is hard to read multiple backtrace", and I worry to open a new issue with unrelated multiple backtraces.
There was a problem hiding this comment.
And how does making them run the formatter for each file which is broken help that? This is a kludge at best.
There was a problem hiding this comment.
They will see new message "please report a bug including the contents of it" by running formatter for each file, it helps them, and I think my solution is kludge enough.
There was a problem hiding this comment.
I think we need another opinion on this.
| end | ||
| end | ||
| end | ||
|
|
||
| private def couldnt_format(file) | ||
| private def couldnt_format(file, ex) | ||
| STDERR << "Error:".colorize(:red).toggle(@color) << ", " << | ||
| "couldn't format " << file << ", please report a bug including the contents of it: https://github.com/crystal-lang/crystal/issues" |
There was a problem hiding this comment.
how about
the contents of the file and this backtrace
?
There was a problem hiding this comment.
Reverted for now.
makenowjust
force-pushed
the
fix/crystal-format/show-backtrace-when-bug-found
branch
from
19b2c3d
to
1563f3c
Compare
|
Resolved conflicts. |
|
ping. I desire to merge this. |
| STDERR << "Error:".colorize(:red).toggle(@color) << ", " << | ||
| "couldn't format " << file << ", please report a bug including the contents of it: https://github.com/crystal-lang/crystal/issues" | ||
| private def couldnt_format(file, ex = nil) | ||
| STDERR << "Error:".colorize(:red).toggle(@color) << ", " |
There was a problem hiding this comment.
Why is there a colon and a comma here? I think that's wrong.
|
Please merge! |
|
@makenowjust Could you rebase against master? CI is failing in Circle. |
backtrace is useful to fix a bug.
makenowjust
force-pushed
the
fix/crystal-format/show-backtrace-when-bug-found
branch
from
59c72a6
to
0cacfa0
Compare
|
rebased |
makenowjust
deleted the
fix/crystal-format/show-backtrace-when-bug-found
branch
backtrace is useful to fix a bug.