envy icon indicating copy to clipboard operation
envy copied to clipboard

Question: Semantics of `envMaybe`

Open StevenXL opened this issue 3 years ago • 2 comments

envMaybe indicates that it will return Nothing if the environmental variable is not set, but it also returns Nothing if the environmental variable is set but is not parsable into the domain data type - i.e., fromVar returns Nothing.

You can construct a function with the the same type but different semantics, in that it will return Nothing if the variable is not set, but if the variable is set, it will attempt to parse it and error out if it cannot.

I am using such a function in my own project, and it looks like this (though I am sure there are more idiomatic ways to write this):

parseEnv :: Var a => String -> Parser (Maybe a)
parseEnv envName = do
  let parseError = throwError $ "Unable to parse " <> envName
  mStr <- optional (env envName) `catchError` (\_ -> pure Nothing)
  maybe (pure Nothing) (maybe parseError (pure . Just) . fromVar) mStr

Should something like this be added to the library, or is it easy enough to build with the current functions and I did more work than necessary?

Addendum

The haskell script below showcased the the following behavior:

  1. When "ENV_INT" is not found in the environment, it returns Nothing for the Int field of the Environment data type.
  2. When "ENV_INT" is found in the environment, but not parsable into an Int, then the parser fails.
  3. When "ENV_INT" is found in the environment, and parsable into an Int, then the parser succeeds with a Just.
#!/usr/bin/env stack
-- stack --resolver lts-14.7 script

{-# LANGUAGE DeriveGeneric       #-}
{-# LANGUAGE ScopedTypeVariables #-}

import           Control.Applicative
import           Control.Monad.Error.Class
import           GHC.Generics
import           System.Envy
import           Text.Read

newtype Custom = Custom { unCustom :: Int } deriving (Show, Eq)

instance Var Custom where
  toVar = toVar . unCustom
  fromVar s = Custom <$> readMaybe s


data Environment = Environment { envInt :: Maybe Int, envString :: Maybe String, envCustom :: Maybe Custom } deriving (Show, Eq, Generic)

instance FromEnv Environment where
  fromEnv _ = Environment <$> parseEnv "ENV_INT" <*> parseEnv "ENV_STRING" <*> parseEnv "ENV_CUSTOM"

parseEnv :: Var a => String -> Parser (Maybe a)
parseEnv envName = do
  let parseError = throwError $ "Unable to parse " <> envName
  mStr <- optional (env envName) `catchError` (\_ -> pure Nothing)
  maybe (pure Nothing) (maybe parseError (pure . Just) . fromVar) mStr

emptyEnv :: Environment
emptyEnv = Environment { envInt = Nothing, envString = Nothing, envCustom = Nothing}

main :: IO ()
main = do
  env <- decodeEnv :: IO (Either String Environment)
  print env

StevenXL avatar Jul 17 '20 04:07 StevenXL

I've contemplated making an Either EnvError a type, defined as:

type FieldName = String
data EnvError = NotFound String | ParseError FieldName String

This would mean making fromVar return an Either String a, and using readEither instead. But I've found the error messages are typically not helpful in this scenario.

> import Text.Read
> readEither "foo" :: Either String Int
Left "Prelude.read: no parse"

One issue with parse failures is that they rarely happen, and when they do happen it represents a fault of the user typically (accidentally attempting to treat a String as an Int). In such cases the user typically aligns the types correctly and never experiences the error again. Since the environment is dynamic its almost impossible for envy to know what to expect. One use case where this may be justified is the continual setting / getting of variables to an environment, but I've never found anyone who could justify this use case.

Despite the error messages being unhelpful, I can see the problem with returning Nothing in both the case where the variable doesn't exist, and when a parse failure has occurred. Instead of returning the parse failure message, we could return a message like.

msg :: String
msg = "Failed to parse variable: " ++ envName ++ " into type: " ++ show (typeOf (undefined :: Int))

We could make the above change w/o needing to use an Either, but just by defining envMaybe in terms of env. This will give us a correct throwError in the case a parse failure occurs, and will return a Nothing for the non-existence case. This arguably is what we should have done in the first place.

------------------------------------------------------------------------------
-- | Environment variable getter returning `Maybe`
envMaybe :: Var a
         => String           -- ^ Key to look up.
         -> Parser (Maybe a) -- ^ Return `Nothing` if variable isn't set.
envMaybe key = do
  val <- liftIO (getEnv key)
  return $ case val of
   Nothing -> Nothing
   Just x -> Just <$> env x -- defines 'envMaybe' in terms of 'env'

I would accept a PR that had this.

dmjio avatar Jul 17 '20 08:07 dmjio

@StevenXL tldr; yes, something like should be added to the library imo. Modifying envMaybe to call env is the right approach imo.

dmjio avatar Jul 18 '20 10:07 dmjio

envMaybe doesn't report reading error (version 2.1.1.0)

yaitskov avatar Mar 03 '23 04:03 yaitskov