Add Process::Status#description
#15468
Conversation
straight-shoota
force-pushed
the
feat/process-exit_reason-message
branch
from
fe444fa to
ee86ce6
Compare
src/process/status.cr
Outdated
| description = exit_reason.description | ||
|
|
||
| if description.same?(SIGNAL_REASON_DESCRIPTION) && (signal = exit_signal?) | ||
| if signal.kill? | ||
| "Process was killed" | ||
| else | ||
| "Process received and didn't handle signal #{signal}" | ||
| end | ||
| else | ||
| description | ||
| end |
There was a problem hiding this comment.
Suggestion: maybe we can directly ask exit_reason? We'd also have no need for the SIGNAL_REASON_DESCRIPTION constant.
| description = exit_reason.description | |
| if description.same?(SIGNAL_REASON_DESCRIPTION) && (signal = exit_signal?) | |
| if signal.kill? | |
| "Process was killed" | |
| else | |
| "Process received and didn't handle signal #{signal}" | |
| end | |
| else | |
| description | |
| end | |
| if exit_reason.signal? && (signal = exit_signal?) | |
| if signal.kill? | |
| "Process was killed" | |
| else | |
| "Process received and didn't handle signal #{signal}" | |
| end | |
| else | |
| exit_reason.description | |
| end |
There was a problem hiding this comment.
Oh yeah. That makes me realize that the signal.kill? branch can never be reached here. This needs a bit of a different control flow.
There was a problem hiding this comment.
Or maybe we just leave the message for Signal::Kill as is: Process terminated abnormally instead of a specialized Process was killed.
Related: #15268
There was a problem hiding this comment.
It would be nice to have, but if it's complex to convey, let's skip 👍
There was a problem hiding this comment.
No, it's super easy. Just adding another branch with exit_signal?.try(&.kill?).
I'm just wondering if it makes sense to have a different message just for Signal::KILL when it is otherwise treated the same as ABORT and QUIT.
There was a problem hiding this comment.
I'd say it's worth distinguishing these two cases.
There was a problem hiding this comment.
Probably not. Process received and didn't handle signal KILL is explicit enough.
There was a problem hiding this comment.
The default description fo Signal::KILL is ExitReason::Aborted.description which is Process terminated abnormally.
This is the current state of this PR. See specs.
| # process.wait.description | ||
| # # => "Process received and didn't handle signal TERM (15)" |
There was a problem hiding this comment.
That IMO would make it clearer:
| # process.wait.description | |
| # # => "Process received and didn't handle signal TERM (15)" | |
| # process.wait.description # => "Process received and didn't handle signal TERM (15)" |
In the compiler we have a neat feature that shows a human readable description when a process process exited with an abnormal status.
This patch moves this as a gernal tool into stdlib as
Process::Status#description.The implementation is split into two parts:
Process::ExitReason#descriptionprovides the static description for most exit reasonsProcess::Status#descriptionenhances that description with details about the termination signal when it's a signal exit without further specification.