-
Notifications
You must be signed in to change notification settings - Fork 213
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
Add {From,To}HttpApiData
instances for Iso8601Range
.
#544
Conversation
48b9cf7
to
510fd5b
Compare
(mt1, mt2) <- arbitrary | ||
let (mv1, mv2) = | ||
if all isJust [mt1, mt2] | ||
then (min mt1 mt2, max mt1 mt2) |
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.
Not sure to get this one? This seems to be forbidding mt1
to be greater than mt2
🤔 ? (whereas that's a valid case for the range..)
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.
Not sure to get this one? This seems to be forbidding
mt1
to be greater thanmt2
thinking ? (whereas that's a valid case for the range..)
Hmm. I don't see how that's a valid case.
From my understanding of Iso8601Range v1 v2
:
v1
is an optional earliest time.v2
is an optional latest time.
So this Arbitrary
instance is designed to only generate the following cases:
Iso8601Range Nothing Nothing
Iso8601Range Nothing (Just t)
Iso8601Range (Just t) Nothing
Iso8601Range (Just t1) (Just t2)
where(t1 <= t2)
.
Perhaps I have misunderstood?
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.
Not sure to get this one? This seems to be forbidding
mt1
to be greater thanmt2
thinking ? (whereas that's a valid case for the range..)
I've just read your later comment. I'll update the code to support both ascending and descending ranges.
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.
@KtorZ Now updated to allow ranges in both ascending and descending order.
parseTimeM False defaultTimeLocale basicUtc $ T.unpack timeText | ||
|
||
validRange :: Maybe UTCTime -> Maybe UTCTime -> Bool | ||
validRange mt1 mt2 = fromMaybe True $ (<) <$> mt1 <*> mt2 |
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.
Ah! Found it!
This is false, the order between mt1
and mt2
conveys the ordering (ASC vs DESC) of the range, cf:
-
20190227T000000Z-20200227T000000Z
: means all transaction between 2019-02-27 and 2020-02-27, in ascending order. -
20200227T000000Z-20190227T000000Z
: means all transaction between 2020-02-27 and 2019-02-27, in descending order.
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.
Ah! Found it!
This is false, the order betweenmt1
andmt2
conveys the ordering (ASC vs DESC) of the range, cf:* `20190227T000000Z-20200227T000000Z`: means all transaction between 2019-02-27 and 2020-02-27, in ascending order. * `20200227T000000Z-20190227T000000Z`: means all transaction between 2020-02-27 and 2019-02-27, in descending order.
Okay, that makes sense. I'll update the code to allow both ascending and descending ranges.
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.
@KtorZ Now updated to allow ranges in both ascending and descending order.
964adc6
to
f1987b6
Compare
f1987b6
to
1fac62c
Compare
dd524e6
to
ec2a083
Compare
1fac62c
to
05f7b05
Compare
…scending. A range in *ascending* order is `Iso8601Range t1 t2` for which `t1 < t2`. A range in *descending* order is `Iso8601Range t1 t2` for which `t1 > t2`.
@jonathanknowles is this ready for final review ? :-) |
05f7b05
to
7494c66
Compare
Nearly ready! Will let you know when finished. |
{From,To}HttpApiData
instances for Iso8601Range
.{From,To}HttpApiData
instances for Iso8601Range
.
0f4a70b
to
0f97181
Compare
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.
Cool! Small remarks to maybe use maybeToEither
-- | ||
-- Example textual representation: | ||
-- | ||
-- > "name *-*" |
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.
very nice instructions describing how to use it 💯
where | ||
expectedPrefix = T.pack $ symbolVal (Proxy @name) | ||
|
||
guardB :: Bool -> String -> Either TextDecodingError () |
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.
probably there are already functions for that
guardB :: Bool -> String -> Either TextDecodingError () | ||
guardB b err = if b then Right () else raise err | ||
|
||
guardM :: Maybe a -> String -> Either TextDecodingError a |
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.
looks like : https://hackage.haskell.org/package/either-5.0.1.1/docs/Data-Either-Combinators.html#v:maybeToRight
Issue Number
#466
Overview
I have:
{From,To}Text
instances forIso8601Range
.{From,To}Text
instances forIso8601Range
.{From,To}Text
instances forIso8601Range
.{From,To}HttpApiData
instances forIso8601Range
.HttpApiData
instances forIso8601Range
.