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.

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

NonEmpty ToSchema instance #141

Closed
wants to merge 7 commits into from
Closed

Conversation

parsonsmatt
Copy link
Contributor

This would be nice to have :) @lucasdicioccio put this in the #93 issue. It seems good to me so I'm opening a PR for it and will be willing to make further changes.

@fizruk
Copy link
Member

fizruk commented Mar 7, 2018

Apparently your branch is out-of-date. Apart from that, I'm happy with the patch!

@phadej you have any comments?

@parsonsmatt
Copy link
Contributor Author

The doctests are failing locally for some reason, but aren't displaying any output 🤷‍♂️

I had to vendor an Arbitrary (NonEmpty a) instance because quickcheck removed it for 2.10.

@phadej
Copy link
Collaborator

phadej commented Mar 7, 2018

Use quickcheck-instances

swagger2.cabal Outdated
@@ -96,6 +98,9 @@ test-suite spec
, unordered-containers
, vector
, lens
if !impl(ghc >= 8.0)
build-depends: semigroups >= 0.18.3 && <0.19
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to specify bounds in test-suite (as they are inherited from library). This helps if/when semigroups-0.19 is released.

@@ -260,3 +262,13 @@ instance Arbitrary ZonedTime where

instance Arbitrary UTCTime where
arbitrary = UTCTime <$> arbitrary <*> fmap fromInteger (choose (0, 86400))

#if MIN_VERSION_QuickCheck(2,10,0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parsonsmatt
Copy link
Contributor Author

Requested changes done :) Thanks!

@fizruk
Copy link
Member

fizruk commented Mar 9, 2018

@phadej are you fine with changes? I can rebase and merge.

instance Arbitrary T.Text where
arbitrary = T.pack <$> arbitrary

instance Arbitrary TL.Text where
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of this gone <3

@phadej
Copy link
Collaborator

phadej commented Mar 9, 2018

@fizruk let's btw wait until we get dependencies to compile with GHC-8.4.1 and make the release then. Do you have some pending changes, or will master be ok to release?

@fizruk
Copy link
Member

fizruk commented Mar 9, 2018

@phadej there's one more PR (#145), but it's cosmetic.
I'll rebase and merge this PR then and then we can wait for a green light for GHC 8.4.1.

@phadej
Copy link
Collaborator

phadej commented Mar 9, 2018

@fizruk good plan

fizruk added a commit that referenced this pull request Mar 9, 2018
NonEmpty ToSchema instance

* parsonsmatt-master:
  Remove unused instance code
  use quickcheck-instances
  Fix tests
  Add semigroups build dep
  (squash-me) support GHC 7.x
  Add ToSchema NonEmpty instance for GHC 8.
@fizruk
Copy link
Member

fizruk commented Mar 9, 2018

Rebased and merged manually in 22e975e.

@fizruk fizruk closed this Mar 9, 2018
@parsonsmatt
Copy link
Contributor Author

Thanks! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants