Refactor entity constraint parsing in Quasi module#1315
Refactor entity constraint parsing in Quasi module#1315parsonsmatt merged 10 commits intoyesodweb:masterfrom
Conversation
| just1 :: (Show x) => Maybe x -> Maybe x -> Maybe x | ||
| just1 (Just x) (Just y) = error $ "expected only one of: " | ||
| `mappend` show x `mappend` " " `mappend` show y | ||
| just1 x y = x `mplus` y |
There was a problem hiding this comment.
I went with using Semigroups/Monoids for this change, but this implementation of just1 which is used in the semigroup instance makes it unlawful. I could re-write this to not use these typeclasses, but I thought it was a nice refactoring (means I could use foldMap, mempty).
Alternatively this could use something like First or Last, though it is probably a nicer user experience to receive this error than for it to silently proceed using one of the values over the other in the case of duplicate values. In fact that is definitely not what we would want.
It may be that now I have done an initial refactor, I could tweak things further again to not have unlawful instances, and use something instead foldMap / mempty
There was a problem hiding this comment.
Hmm! I like using the Monoid and Semigroup instances here. That's neat. But I don't like having a Semigroup operation that will error on a relatively common case - that seems like a footgun.
I feel like a type that better represents this is something like:
data SetOnce a
= SetOnce a
| NotSet
| SetManyTimes (NonEmpty a)
instance Semigroup (SetOnce a) where
NotSet <> a = a
SetOnce a <> NotSet = SetOnce a
SetOnce a <> SetOnce b = SetManyTimes (a :| [b])
SetOnce a <> SetManyTimes as = SetManyTimes (NEL.cons a as)
a@(SetManyTimes xs) <> NotSet = a
SetManyTimes xs <> SetOnce a = SetManyTimes (xs `snoc` a)
SetManyTimes xs <> SetManyTimes ys = SetManyTimes (xs <> ys)
instance Monoid (SetOnce a) where
mempty = NotSetRendering/erroring can then happen outside of the code.
| just1 :: (Show x) => Maybe x -> Maybe x -> Maybe x | ||
| just1 (Just x) (Just y) = error $ "expected only one of: " | ||
| `mappend` show x `mappend` " " `mappend` show y | ||
| just1 x y = x `mplus` y |
There was a problem hiding this comment.
Hmm! I like using the Monoid and Semigroup instances here. That's neat. But I don't like having a Semigroup operation that will error on a relatively common case - that seems like a footgun.
I feel like a type that better represents this is something like:
data SetOnce a
= SetOnce a
| NotSet
| SetManyTimes (NonEmpty a)
instance Semigroup (SetOnce a) where
NotSet <> a = a
SetOnce a <> NotSet = SetOnce a
SetOnce a <> SetOnce b = SetManyTimes (a :| [b])
SetOnce a <> SetManyTimes as = SetManyTimes (NEL.cons a as)
a@(SetManyTimes xs) <> NotSet = a
SetManyTimes xs <> SetOnce a = SetManyTimes (xs `snoc` a)
SetManyTimes xs <> SetManyTimes ys = SetManyTimes (xs <> ys)
instance Monoid (SetOnce a) where
mempty = NotSetRendering/erroring can then happen outside of the code.
| pure <$> takeUniq ps "" defs (n : rest) | ||
| } | ||
| _ -> | ||
| mempty |
There was a problem hiding this comment.
This is much nicer 👍🏻
danbroooks
force-pushed
the
quasi-refactor-entity-constraints
branch
from
1f539dd to
3318d91
Compare
2 participants
Before submitting your PR, check that you've:
@sincedeclarations to the Haddockstylish-haskellon any changed files..editorconfigfile for details)After submitting your PR:
(unreleased)on the ChangelogA further contribution towards #1156, addresses the fold referenced in that issue by creating a type that holds the various possible constraints, and can be built up and merged into one. I have used
Monoids /Semigroups however the instances are not lawful. I will put some further detail about this as a comment in the relevant area in the PR.I have also renamed
splitExtrastoparseEntityFieldsas this is more descriptive I think. This function is called when the fields of an entity are extracted, the first step being to split the'extra'tokens from the rest of the entity fields which are added to a value of typeM.Map Text [ExtraLine], where the key is the entity name and its[ExtraLine]values are the entity fields.