Raise Thor::Error instead of exit
#2207
Conversation
lib/tapioca/commands/abstract_dsl.rb
Outdated
| @@ -131,15 +131,22 @@ def load_application | |||
|
|
|||
| sig { returns(Tapioca::Dsl::Pipeline) } | |||
| def create_pipeline | |||
| error_handler = if @lsp_addon | |||
| ->(error) { | |||
| say(error, :bold, :red) | |||
There was a problem hiding this comment.
This is an adjacent change to show errors in lsp output. say_error messages aren't visible in the logs, I assume due to stderr, but we'd like to have them.
There was a problem hiding this comment.
Let's remove the styling symbols? I assume they do nothing in LSP mode.
| if errors.any? | ||
| errors.each do |msg| | ||
| report_error(msg) | ||
| end |
There was a problem hiding this comment.
Given this usage I don't think we should raise inside report_error. We'll also be fixing a small bug.
| @@ -78,6 +78,7 @@ def run(&blk) | |||
| No classes/modules can be matched for RBI generation. | |||
| Please check that the requested classes/modules include processable DSL methods. | |||
| ERROR | |||
| raise Thor::Error, "" | |||
There was a problem hiding this comment.
| raise Thor::Error, "" | |
| raise Thor::Error, "Exiting" |
(or something like that)
There was a problem hiding this comment.
We don't know if we're exiting. I left it empty thinking that the error message above is sufficient. Do you think it's worth adding a secondary message?
There was a problem hiding this comment.
Oh, is the argument displayed to the user?
(not a blocker, I was suggesting it just to ease possible future troubleshooting. If we saw that without any backtrace then it could be pain to track where it came from).
There was a problem hiding this comment.
Yes it's displayed without the stack trace: https://github.com/rails/thor/blob/3178667e1727504bf4fb693bf4ac74a5ca6c691e/lib/thor/base.rb#L586.
Right now we're getting an extra new line in the output due to this raise which isn't ideal 😬
`Thor::Error` is rescued by thor and we can control its behaviour using `exit_on_failure?` https://github.com/rails/thor/blob/3178667e1727504bf4fb693bf4ac74a5ca6c691e/lib/thor/base.rb#L587
KaanOzkan
force-pushed
the
ko/addon-exit
branch
from
83db825 to
cfe19ce
Compare
We now output an extra line due to the new raise. We can use an `includes` check instead of `equals` to prevent the error in the future
KaanOzkan
force-pushed
the
ko/addon-exit
branch
from
cfe19ce to
145834d
Compare
I opened #2208, feel free to make suggestions in the issue |
KaanOzkan
force-pushed
the
ko/addon-exit
branch
from
145834d to
d14f363
Compare
|
We have a CI failure due to Sorbet payload, it might be the github action cache or something is preventing bumping Sorbet in some workflows. I'm going to go ahead since this fix is time sensitive. |
Motivation
Exiting in add-on mode causes progress notifications to not be cleaned up
Implementation
Raise a
Thor::Errorinstead. It's rescued by thor and we control its behaviour usingexit_on_failure?hereTests