-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecating the Process exit_status method in favor of exit_code. #8647
base: master
Are you sure you want to change the base?
Conversation
IMO this is still useful in a platform-specific context and shouldn't be
removed, maybe just renamed to platform_exit_status or similar?
…On Fri, Jan 3, 2020, 7:33 PM Sijawusz Pur Rahnama ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/process/status.cr
<#8647 (comment)>:
> @@ -25,6 +22,13 @@ class Process::Status
Signal.from_value(signal_code)
end
+ # Platform-specific exit status code, which usually contains either the exit code or a termination signal.
+ # The other `Process::Status` methods extract the values from `exit_status`.
+ @[Deprecated("Use `Process::Status#exit_code`")]
IIRC it should
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8647?email_source=notifications&email_token=AAM4YSMHQCMEQJDTKJNADUDQ37RP3A5CNFSM4KCTCAZKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQVIFNA#discussion_r363007780>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM4YSNHA534RLC5TKEMZ3TQ37RP3ANCNFSM4KCTCAZA>
.
|
@refi64 In the other thread there was a mention of possibly renaming it. I'm cool with that option too. I'll wait for some more feedback before I change anything though. |
I think it can be removed, it's easy enough to bitshift it back if required. |
That's not really the same. Since exit_code is undefined if the process
didn't exit normally, you'd have to have a chain of ifs tested if it was
exited, signaled, or stopped, and then reconstructing the original exit
status.
…On Sat, Jan 4, 2020, 6:26 PM Stephanie Hobbs ***@***.***> wrote:
I think it can be removed, it's easy enough to bitshift it back if
required.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8647?email_source=notifications&email_token=AAM4YSNZPVJRZGUMSVGGI7LQ4ESK3A5CNFSM4KCTCAZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIDDLYA#issuecomment-570832352>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM4YSM27TVUD4RZDAJEMO3Q4ESK3ANCNFSM4KCTCAZA>
.
|
Simple question: How's this implemented in other languages? We don't need to reinvent such basic things, just look what others are doing and pick the best one. |
I'm down for any suggestions here. I think the main thing is that it's clear what one does versus another. It almost feels like these two method names are reversed. I tried looking at a few other languages, but trying to decipher low level process stuff is a little over my head. |
In Go lang, ExitCode() returns the exit code of the exited process, or -1 if the process hasn't exited or was terminated by a signal. In Rust, ExitStatus has method code, that Returns the exit code of the process, if any. On Unix, this will return None if the process was terminated by a signal; In Nim, Process method peekExitCode Return -1 if the process is still running. Otherwise the process' exit code. On posix, if the process has exited because of a signal, 128 + signal number will be returned. That +128 is similiar what all Unix shells do. for example in Bash documented here You can find similar note in Java native sources (Solaris implementation):
But since we have wrapping Process::Status class, we can easily deprecate exit_status method, as proposed, so +1 for that. Any other informations encoded in status can be easily accessed by other methods like exit_signal and normal_exit?. Is for discussion what to return from exit_code method if exited abnormally, but I think it is subject for another PR? |
Thanks for looking up that info @jan-zajic |
We should probably follow Go and Rust in only returning an exit code if there is actually one and not tread a signal indicator as an exit code. This may be fine for more platform-specific implementations, but for a portable language it feels wrong when a child doesn't exit but the process status reports an exit code. Go's implementation also provides access to system-specific information, such as extracting the singal number or whether the process was core dumped. The API is hidden in
I'd prefer to combine deprecation of |
I think it's very clear that we shouldn't expose So, just due to this, it's not 100% obvious that this is a good change, as it might direct people towards checking |
If I was making this decision on my own, I'd merge this PR immediately and then also make |
I've had to rebuild exit status values before because a language doesn't expose it, it's a pretty painful experience and I don't really see any loss from having e.g. platform_exit_status. |
Having explored the full variety of exit statuses and how poorly they are covered by the convenience methods, now I'm not so sure that we are ready to deprecate this. In addition to what @refi64 said. |
issue: |
I originally ran in to some confusion when using
exit_status
and getting a0
even though the program was raising an exception. It turned out thatexit_code
was the correct method here.As per @RX14 comment: #8381 (comment) I have deprecated the
exit_status
method in favor of just usingexit_code
.Fixes #8381