-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
[#50] Add decode.encode = id tests #173
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 tests look pretty good! I have couple suggestions. Need to look carefully, where is the error...
test/Test/Toml/BiCode/Property.hs
Outdated
genInt = Gen.int (Range.constant 0 256) | ||
|
||
genWord :: Gen Word | ||
genWord = Gen.word (Range.constant 1 256) |
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.
You can use constantBounded
generator for Int
and Word
to avoid specifying boundaries manually. Unfortunately, won't fork for Double
and Float
, they don't have instances...
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.
Thanks!
Also, do you think that it's better to move these generators to Gen
module?
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.
@vrom911 Yeah, I think it's a good idea to move general-purpose generators (like genWord
) into single module, because they are probably going to be reusable in several places (for example, I can see that BiMap.Property
uses most of the same generators. It's a good idea to keep all basic generators in single place 🙂
deriving (Show) | ||
|
||
instance Eq BigTypeNewtype where | ||
(BigTypeNewtype a) == (BigTypeNewtype b) = zonedTimeToUTC a == zonedTimeToUTC b |
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 idea to move ZonedTime
inside newtype
! With this trick you can minimize the size of Eq
instance and test both diwrap
and zonedTime
!
, btText :: Text | ||
, btString :: String | ||
, btBS :: ByteString | ||
, btLazyBS :: L.ByteString |
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.
Lol, I just noticed that in tomland
we have Text
, ByteString
, LByteString
and String
but not LText
. Heh, that's awkward. Probably need to add LText
to tomland
as well. I've created issue for this:
02fefac
to
8a631b3
Compare
@kowainik/dev it's ready for review! |
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.
This looks good to me! I have only minor suggestion 🙂 Otherwise this is huge work! I also appreciate the refactoring 🙂
, genInteger | ||
, genDouble |
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 like separation of reexports in this export section! 😍
Co-Authored-By: vrom911 <vrom911@gmail.com>
Resolves #50