search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.42k stars 1.62k forks source link

Public constructor for `Process::Status` #13016

Open straight-shoota opened 1 year ago

straight-shoota commented 1 year ago

Process::Status currently has no public constructor, so it's impossible to explicitly create an instance. Instances can only be acquired from Process#wait. There is an undocumented internal constructor of course, but it's also non-portable.

I don't think there are particularly many use cases for constructing a Process::Status instance. But it's definitely useful for writing tests. We do that in stdlib specs. And I'm sure it would be useful for user code that deals with process interfaces. It's an easy feat for completeness and we need it to be constructable anyway.

The internal constructor depends on the platform and it's not easily usable because it receives a bit-encoded value on POSIX platforms. In stdlib specs we use this helper method to provide a portable interface: https://github.com/crystal-lang/crystal/blob/f2c0b3e2efcac49eb6f82ed946bc3d918c0a4273/spec/std/process/status_spec.cr#L3-L9

Implementing this is technically easy but we still have to navigate some corners.

We can easily rename the internal constructors used by Process#wait. But the current semantics of the internal constructors are still available and might be used, despite being undocumented. If we change the semantics of Process.new(Int32), this would be a silent change. So it might be best to make it fail hard, either be removing it entirely or adding a deprecation warning. Then at some point later we can add it again with new, portable semantics.

For the mean time we could already introduce an alternative constructor with a different signature. Process::Status.[] would suggest itself for that. It would have a double effect of making the output of Process::Status#inspect as suggested in #13012 valid code: Process::Status[0], Process::Status[Signal::HUP].

HertzDevil commented 1 year ago

The constructors could also be in the form of Status.new(*, normal : Int) or Status.normal(status : Int), it is easy to avoid any possibility of overloading or overriding the existing no-doc constructor

HertzDevil commented 1 year ago

What would the signal constructor look like on Windows or perhaps WebAssembly? NotImplementedError?

straight-shoota commented 1 year ago

Yes, that would probably be good for a start.

I was wondering if we could at some point have a fully portable implementation of Process::Status, similar to Path which supports representing both Windows and POSIX paths on any platform. I guess the use cases wouldn't be that big for Process::Status - mostly for applications that execute remote processes on mixed operating systems. That's probably too niche to warrant a generic stdlib implementation. But perhaps it could at least help improve the Crystal representation. I figure it might be a bit simpler if we had the same portable representation and platform specific code would only need to translate to that generic representation. Not sure if this is a good idea after all, but it's definitely not urgent.