-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#63] Add SeriesP class #87
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results are really outstanding! I like how it's implemented 👍 I juts want more examples in documentation and one minor cosmetic change.
instance (KnownRatName unit) => SeriesP '[unit] where | ||
seriesP :: forall (someUnit :: Rat) . KnownRatName someUnit | ||
=> String -> Maybe (Time someUnit) | ||
seriesP "" = Just $ Time 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case (parsing empty string) should be in documentation. It's a hard problem, how we want to handle empty string. But this is not important at the moment. What's important is not to have undocumented behavior.
>>> seriesP @'[Hour, Second, Millisecond] @Minute "90s" | ||
Just (1+1/2m) | ||
|
||
>>> seriesP @'[Hour, Second] @Second "11ns" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add examples with something like seriesP @'[Hour, Minute]
where you have 1+1/2h
and 1+1/2m
and 1h1+1/2m
?
src/Time/Series.hs
Outdated
maybeT = readMaybeTime @unit $ num ++ u | ||
in case maybeT of | ||
Nothing -> seriesP @(nextUnit ': units) str | ||
Just _ -> liftA2 (+) maybeT (seriesP @(nextUnit ': units) nextStr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't like ignoring result of Just
here. Here you perform manual unwrapping of maybeT
and then you pass maybeT
to liftA2
function which will do this unwrapping again. It looks better to me to use fmap
here, like
Just t -> (t+) <$> seriesP ...
Resolves #63