[#172] Add Semigroup and Monoid instances to Validation#175
[#172] Add Semigroup and Monoid instances to Validation#175vrom911 merged 4 commits intokowainik:masterfrom
Conversation
chshersh
left a comment
There was a problem hiding this comment.
@mauriciofierrom Thanks for your work! This is a useful addition to the library 👍 You asked about documentation and how it can be improved. And here are my thoughts. Validation has many instances with non-obvious behaviour. It's not clear how the instances are implemented without looking at the code. And all these instances (Applicative, Semigroup, Monoid, Alternative) solve different problems. So I would like to have more expanded top-level Haddock for this module that describes use cases and recommends instances. Something like that:
- If you want to validate data and return combined errors or result of sequential functional applications, use
Applicativeinstance. This instance returnsSuccessonly when all values areSuccess. - If you want to return the combined result of all errors or the first successful result, use
Alternative. - If you're going to combine errors and validation results monoidally, use
Semigroup/Monoidinstance. UseFirst/Last/[]monoids to specify how you want to combine errors/results. This instance returnsSuccessonly when all values areSuccess.
Something like that. Just a short description of what instances and functions users need to use when they want specific behaviour. Does this make sense?
CHANGELOG.md
Outdated
| * [#155](https://github.com/kowainik/relude/issues/155): | ||
| Implement `Relude.Extra.Foldable` module. | ||
| * Re-export `GHC.Float.atan2`. | ||
| * Add `Monoid` and `Semigroup` instances for `Validation` type |
There was a problem hiding this comment.
Thanks for updating CHANGELOG as well! Could you also write it in the format with the link to the corresponding issue number?
There was a problem hiding this comment.
Oops, sorry! I'll change this.
src/Relude/Extra/Validation.hs
Outdated
| -} | ||
|
|
||
| instance (Semigroup e, Semigroup a) => Semigroup (Validation e a) where | ||
| x <> y = (<>) <$> x <*> y |
There was a problem hiding this comment.
I would love to have several changes in this implementation:
- Add explicit function signature in the spirit of all instances.
- Use
liftA2. Basically, this function can be implemented simpler:(<>) = liftA2 (<>)
- Add
{-# INLINE #-}pragmas to<>andmempty
There was a problem hiding this comment.
Sorry, I'll change this as well.
src/Relude/Extra/Validation.hs
Outdated
| Failure ["Not correct","Not correct either"] | ||
|
|
||
| >>> liftA2 (+) a b | ||
| >>> (+) <$> a <*> b |
There was a problem hiding this comment.
The fact that we don't implement liftA2 doesn't mean that we cannot use it. I think that it's completely okay to use liftA2 in this documentation.
|
Yes, that make sense. I'll work on that. Thanks for your explanation! |
chshersh
left a comment
There was a problem hiding this comment.
Thanks for your effort! This looks great 🙂 I think documentation always can be improved but this is already very useful 👍
Resolves #172.
liftA2custom definition.SemigroupandMonoidinstances toValidationtype.Validationmodule with use cases.Checklist:
HLint
hlint.dhallaccordingly to my changes (add new rules for the new imports, remove old ones, when they are outdated, etc.)..hlint.yamlfile (see this instructions).General
stylish-haskellfile.[ci skip]text to the docs-only related commit's name.