process icon indicating copy to clipboard operation
process copied to clipboard

Show instance for System.Process.CreateProcess and CmdSpec

Open elaforge opened this issue 9 years ago • 6 comments

Looking at the source there doesn't seem to be any particular reason to omit Show. My best guess is that it's since some details are platform dependent, but since Show is basically for debugging, it doesn't seem like a blocker.

Also it would be nice to be able to get a PID from a ProcessHandle. I notice this one is more heavily system specific and it has an awkward mvar but it would be nice to get some kind of indication about which process just died other than the name I started it with.

If there's no fundamental objection I can try a patch.

elaforge avatar Feb 15 '16 05:02 elaforge

I'm cautious about both of these, since:

  1. As you mention, PID is OS-specific to some degree
  2. If we decide to modify the representation of ProcessHandle in the future, it could make it much more difficult to maintain a backwards-compatible PID interface
  3. I'm worried that exposing a Show instance will prevent us from adding new fields in the future that would not be Showable, or expose internals that we may ultimately want to hide

I'm not vetoing the changes, but I do want some caution on it. The one thing that can certainly be done is expose a function in the Internals module for getting a PID.

snoyberg avatar Feb 15 '16 09:02 snoyberg

since Show is basically for debugging

...is it? I was under the impression that opinions differ on the purpose of Show/Read... :-)

hvr avatar Feb 15 '16 09:02 hvr

What else do people use Show for? I know you can do deserialization with Read, but if there's no Read then I assume no one will have that expectation. I also wouldn't expect Show to retain backwards compatibility, especially if the type itself changed. At that point you likely have a API bump anyway.

CreateProcess is just a way to group arguments so it seems likely that each field will continue to correspond directly to an argument. It's not really the place to stash secret internals. And even if it were, I wouldn't be above a custom Show that omits unshowable fields.

As for ProcessHandle, surely it's always going to have a pid in there somewhere for unix. How could we have a function like 'waitForProcess' but somehow not have a pid in the ProcessHandle? And in fact PHANDLE is already directly exposed in Internals, just not a way to get it from a ProcessHandle.

elaforge avatar Feb 15 '16 23:02 elaforge

I just realized that since ProcessHandle is exposed in internals I can already get the PID. A function would just be a convenience.

elaforge avatar Feb 16 '16 00:02 elaforge

Would it also make sense for there to be an Eq instance for CreateProcess and CmdSpec?

dkubb avatar Mar 19 '16 02:03 dkubb

Yeah, don't see why not. I had forgotten about this until now. So I just added a pull request so there's nothing left for me to remember to do.

elaforge avatar Mar 19 '16 03:03 elaforge