Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add {From,To}HttpApiData instances for Iso8601Range. #544

Merged
merged 9 commits into from Jul 19, 2019
Merged

Add {From,To}HttpApiData instances for Iso8601Range. #544

merged 9 commits into from Jul 19, 2019

Conversation

jonathanknowles
Copy link
Member

@jonathanknowles jonathanknowles commented Jul 16, 2019 •
edited

Issue Number

#466

Overview

I have:

  • Added {From,To}Text instances for Iso8601Range.
  • Added roundtrip tests for {From,To}Text instances for Iso8601Range.
  • Added set of negative tests for {From,To}Text instances for Iso8601Range.
  • Added {From,To}HttpApiData instances for Iso8601Range.
  • Added roundtrip tests for HttpApiData instances for Iso8601Range.

@jonathanknowles jonathanknowles self-assigned this Jul 16, 2019
@KtorZ KtorZ mentioned this pull request Jul 16, 2019
Closed
11 tasks
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/api-iso8601-range branch 2 times, most recently from 48b9cf7 to 510fd5b Compare July 17, 2019 03:16
KtorZ
KtorZ reviewed Jul 17, 2019
View reviewed changes
lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs Outdated
(mt1, mt2) <- arbitrary
let (mv1, mv2) =
if all isJust [mt1, mt2]
then (min mt1 mt2, max mt1 mt2)
Copy link
Member

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..)

Copy link
Member Author

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 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?

Copy link
Member Author

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 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.

Copy link
Member Author

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.

lib/core/src/Cardano/Wallet/Api/Types.hs Outdated
parseTimeM False defaultTimeLocale basicUtc $ T.unpack timeText

validRange :: Maybe UTCTime -> Maybe UTCTime -> Bool
validRange mt1 mt2 = fromMaybe True $ (<) <$> mt1 <*> mt2
Copy link
Member

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.

Copy link
Member Author

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.

Okay, that makes sense. I'll update the code to allow both ascending and descending ranges.

Copy link
Member Author

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.

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/api-iso8601-range branch 2 times, most recently from 964adc6 to f1987b6 Compare July 17, 2019 09:13
@jonathanknowles jonathanknowles mentioned this pull request Jul 17, 2019
Merged
5 tasks
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/api-iso8601-range branch from f1987b6 to 1fac62c Compare July 18, 2019 09:25
@jonathanknowles jonathanknowles changed the base branch from master to jonathanknowles/time-text July 18, 2019 09:26
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/api-iso8601-range branch from 1fac62c to 05f7b05 Compare July 18, 2019 09:50
@jonathanknowles jonathanknowles changed the base branch from jonathanknowles/time-text to master July 18, 2019 10:54
@paweljakubas
Copy link
Contributor

paweljakubas commented Jul 19, 2019 •
edited

@jonathanknowles is this ready for final review ? :-)
some tests still missing, right?

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/api-iso8601-range branch from 05f7b05 to 7494c66 Compare July 19, 2019 08:40
@jonathanknowles
Copy link
Member Author

@jonathanknowles is this ready for final review ? :-)
some tests still missing, right?

Nearly ready! Will let you know when finished.

@jonathanknowles jonathanknowles changed the title WIP: Add {From,To}HttpApiData instances for Iso8601Range. Add {From,To}HttpApiData instances for Iso8601Range. Jul 19, 2019
@jonathanknowles jonathanknowles marked this pull request as ready for review July 19, 2019 09:43
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/api-iso8601-range branch from 0f4a70b to 0f97181 Compare July 19, 2019 10:03
paweljakubas
paweljakubas approved these changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@paweljakubas paweljakubas left a 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

lib/core/src/Cardano/Wallet/Api/Types.hs
--
-- Example textual representation:
--
-- > "name *-*"
Copy link
Contributor

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 💯

lib/core/src/Cardano/Wallet/Api/Types.hs
where
expectedPrefix = T.pack $ symbolVal (Proxy @name)

guardB :: Bool -> String -> Either TextDecodingError ()
Copy link
Contributor

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

lib/core/src/Cardano/Wallet/Api/Types.hs
guardB :: Bool -> String -> Either TextDecodingError ()
guardB b err = if b then Right () else raise err

guardM :: Maybe a -> String -> Either TextDecodingError a
Copy link
Contributor

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

@paweljakubas paweljakubas merged commit ae82326 into master Jul 19, 2019
@paweljakubas paweljakubas deleted the jonathanknowles/api-iso8601-range branch July 19, 2019 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ left review comments

@paweljakubas paweljakubas paweljakubas approved these changes

Assignees

@jonathanknowles jonathanknowles

Labels
None yet
Projects
None yet
Milestone
Finalize 'Transactions' API endpoints
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonathanknowles @paweljakubas @KtorZ