Some things that I noticed with a cursory inspection:
- A helper function like the following would be nice
(not just to reuse, but also because it’s not stringly typed, i.e. if you mistype Bool it’ll tell you, it won’t if you mistype "Bool")
{-# LANGUAGE TypeApplications #-}
import Data.Proxy (Proxy (..))
import Data.Typeable (typeOf, typeRep)
valueIsBool :: Value -> Bool
valueIsBool (Value v) = typeOf v == typeRep (Proxy @Bool)
- You seem to use the
go helper in where pattern in places where a case would suffice. AND you don’t have to keep giving arguments to helper functions if they are already top level (i.e. the pair here for check_for_arg, since it’s not recursive.
test :: (String,String) -> HashMap String Value -> Either Error ()
test pair@(k1, k2) m =
case Map.lookup k1 m of
Nothing -> Left $ UnknownFlag k1
Just v -> check_for_arg v >> checkForCompatibility pair v
where
check_for_arg :: Value -> Either Error ()
check_for_arg (Value v)
| valueIsBool v && (not $ null k2) =
Left . FlagSyntax $ k1 <> " must not have arg"
| not (valueIsBool v) && null k2 =
Left . FlagSyntax $ k1 <> " must have arg"
| otherwise = Right ()
- ALSO, most prefer
checkForArg, because snake case is used for C FFI functions, but most importantly because of Haskell’s use of space as semantically significant, the underscores sometimes seem like spaces, so it makes you do a double take that it isn’t check for arg which is a function with two arguments, instead of check_for_arg which is just a function.
myFunc = map (some_underscores_func 5) someList -- compare the following
myFunc = map (some underscores func 5) someList -- similar but very different
myFunc = map (someUnderscoresFunc 5) someList -- very distinct and immediately obvious
- Why
splitOn and then intercalate? If you just want the first one “popped off” do the following:
-- where `delim :: Char`
case break (== delim) xs of
(_, []) -> Nothing
(x, rest) -> Just (x, drop 1 rest)
-
Prefixing underscores to top-level functions is not great, since you won’t get warnings if you never use them (this can lead to dead code accumulating). If you’re using the functions, don’t prefix underscores.
-
I see a lot of fst pair and snd pair when you could just pattern match on the pair and name the first and second part of the pair. Then again, you could have also given two arguments instead of a pair, also avoids unpacking and repacking.
There’s also some higher level decisions about the package that I would have done differently, like why add in the dash to then later remove it again. Just add it in the error messages if it’s for pretty printing. And the result type of checkAndBuild I would make Either Error [(String, Maybe Value)], so you don’t use empty string as a Nothing and combine deduceStrategy and applyChecks so you can immediately return the Value after you do a readMaybe.
A lot of this feels like you could learn from this great article by Alexis King: Parse, don’t validate, because you’re validating a lot (hence the boolean checks and all the Right (), which are a dead giveaway) and parsing would make it more robust and also simpler in the end.
