search

commercialhaskell / rio

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

Export `SimpleApp` constructor and fields #208

Closed lehins closed 4 years ago

lehins commented 4 years ago

Having a data type that contains ProcessContext and LogFunc together with HasLogFunc and HasProcessContext is very useful, but currently the only way to construct it is by using runSimpleApp, which is redundant if both logFunc and context are already available. Also there is already a way to change both saLogFunc and saProcessContext record fields with lens, so I don't see any point of hiding the constructor and record fields from the user.

snoyberg commented 4 years ago

The reason to hide the constructor is so that, if we add an additional field in the future, it's not a breaking change.

lehins commented 4 years ago

I find myself redefining this exact data type and matching instances a few times already, feels very redundant. What is the real chance of a new field appearing in SimpleApp?

lehins commented 4 years ago

Moreover, addition of a new field (whatever that field might be) would likely mean a change in semantics of how runSimpleApp works, which would already be a breaking change.

snoyberg commented 4 years ago

That's a lot of assumptions.

How about adding a mkSimpleApp smart construction, that looks something like:

mkSimpleApp :: MonadIO m => LogFunc -> Maybe ProcessContext -> m SimpleApp

That should fully support extensibility in the future.

lehins commented 4 years ago

I thought about that approach, this of course doesn't give the ability to supply those extra new fields that will appear, but that is the whole point I guess ;) I like it, I'll submit the PR

Also @snoyberg would you oppose me adding this function to RIO.Process?

-- | Look into the `ProcessContext` and see if there is environment variable available there?
lookupEnvVar :: (MonadReader env m, HasProcessContext env) => Text -> m (Maybe Text)
lookupEnvVar envName = Map.lookup envName <$> view envVarsL
snoyberg commented 4 years ago

Yeah, I'd be concerned with that naming, since it doesn't make it clear enough that it's coming from the ProcessContext. Different naming (not sure what) would be fine.