commercialhaskell / rio

A standard library for Haskell
Other
838 stars 54 forks source link

Rio.Process shell (like proc) #245

Closed tulth closed 1 year ago

tulth commented 1 year ago

Just curious, from System.Process.Typed, RIO has proc wrapped nicely, but not shell. Does something prevent adding shell? Perhaps it is because it is hard to cover exceptional behavior like preProcess covers?

RIO shell could be something like this:

shell
  :: (HasProcessContext env, HasLogFunc env, MonadReader env m, MonadIO m, HasCallStack)
  => FilePath -- ^ command to run
  -> (ProcessConfig () () () -> m a)
  -> m a
shell cmd inner = do
  -- can we skip preprocess?  Is it required to get the full path? I would argue not as this may be a compound command and the user asked for a shell command
  -- Fixme: probably should check this wd directory like preProcess
  wd <- view workingDirL 
  envStrings <- view envVarsStringsL

  withProcessTimeLog wd name args
    $ inner
    $ setEnv envStrings
    $ maybe id setWorkingDir wd
    $ P.shell cmd

At least, for withProcessTimeLog, it logs the full command anyway, so it seems it could be rewritten, and have the existing string packer stuff done in another function or on demand if only done once: https://github.com/commercialhaskell/rio/blob/23e7c18bbbb54c9f83d64a5a2878f833eef45746/rio/src/RIO/Process.hs#L401-L405

Apologies if this is a dumb question; I am not a haskell expert.

snoyberg commented 1 year ago

I think you nailed it: it's about the preProcess case. I don't mind including a version of shell like you've described, but we need to include a caveat about how it may work differently from proc in the docs. Generally, my recommendation is to avoid shell whenever possible.

tulth commented 1 year ago

Do you think I should try my hand at a PR for this - or is this something that does not belong in RIO?

snoyberg commented 1 year ago

If someone actually has a use case for it I'm not opposed. But I've never really heard of a good use case for shell over proc.