Skip to content
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

Improve textual representation of Process::Status #13012

Closed
straight-shoota opened this issue Jan 26, 2023 · 6 comments · Fixed by #13044
Closed

Improve textual representation of Process::Status #13012

straight-shoota opened this issue Jan 26, 2023 · 6 comments · Fixed by #13044

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Jan 26, 2023

Process::Status currently has very intricate #to_s and #inspect formats. They're the default implementations inherited from Reference and don't convey much relevant information.

I'm proposing to implement a more meaningful representation.

#to_s currently returns the type name and object id in the typical format of Reference#to_s (e.g. #<Process::Status:0x7f9fd8a17c40>). That doesn't make much sense. The object id is pretty much irrelevant.
Instead, it should simply return a textual representation of the exit code or signal:

process_status(0).to_s           # => "0"
process_status(128).to_s         # => "128"
process_status(Signal::HUP).to_s # => "HUP"

(I'm using a ficticious process_status helper to represent instances with certain values because Process::Status doesn't have a public or easily usable constructor - which should probably be added as well.)

#inspect currently enhances the sparse information output of #to_s by the names of the instance variables and their values. @exit_status is the only ivar and its value is bit encoded, thus not trivial to read. This isn't very useful.
Instead, it should return a textual representation of the exit code or signal, like #to_s but enhanced with the type information of Process::Status for more expressiveness.

process_status(0).inspect           # => "Process::Status[0]"
process_status(128).inspect         # => "Process::Status[128]"
process_status(Signal::HUP).inspect # => "Process::Status[Signal::HUP]"
@HertzDevil
Copy link
Contributor

HertzDevil commented Jan 27, 2023

Shouldn't the second snippet use #inspect?

For Windows it might be helpful to have things like process_status(0xC0000005_u32).to_s # => "STATUS_ACCESS_VIOLATION", but this requires us to define an enum for the possible NTSTATUS values first; yet not all NTSTATUS values really correspond to abnormal exit codes either.

Also I didn't realize Process::Status is a class, I always thought it is a struct already.

@straight-shoota
Copy link
Member Author

Shouldn't the second snippet use #inspect?

Yeah, fixed 😅

@straight-shoota
Copy link
Member Author

Also I didn't realize Process::Status is a class, I always thought it is a struct already.

Yes, it should be. I opened #13015 to track that.

@HertzDevil
Copy link
Contributor

HertzDevil commented Feb 2, 2023

Process::Status[Signal::HUP] is technically ambiguous because it could also represent the signal that stopped a process (i.e. exit_status == 0x017F, like here) rather than terminating it. A similar argument can be made about constructors. I think it is inevitable to support stop statuses in Process::Status too if #inspect is involved.

@straight-shoota
Copy link
Member Author

straight-shoota commented Feb 2, 2023

But currently there's no way to express a stopped signal, right? So I'm thinking this should be left for an enhancement later.
Now we only need make sure that the inspect syntax and constructor signature are compatible to such an amendment. And I think they are. One option could be to use a named parameter like this: Process::Status[stopped: Signal::HUP].

@HertzDevil
Copy link
Contributor

I think they show up from Process#wait with ptrace, although I don't know about the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants