jcpetruzza / barbies

BSD 3-Clause "New" or "Revised" License
92 stars 15 forks source link

Add generic instances for barbies within nested Functors #21

Closed peddie closed 4 years ago

peddie commented 4 years ago

This commit allows generic deriving in code like the following, where multiple (normal-flavoured) Functors are nested around our Barbie:

data Two a = Two a a
  deriving ( Eq, Ord, Show, Generic, Functor, Foldable, Traversable )

data Foo f
  = Foo (f Int)
  | Bar [Two (Foo f)]
  deriving ( Generic, FunctorB, TraversableB )
jcpetruzza commented 4 years ago

Thanks for submitting this! The current limitation to one nested functor is annoying. Ultimately, I'd like to handle arbitrary nesting (briefly discussed in https://github.com/jcpetruzza/barbies/issues/19#issuecomment-533927178) but in the meantime manually adding additional instances will have to be the way to go.

Could I ask you to add some test-cases for the instances you are adding? I'd normally add a type like your Foo above in test/TestBarbies.hs and test/TestBarbiesW.hs and declare all their instances. If you also declare the Show / Eq / Arbitrary instances, you can test the laws hold in test/Spec.hs. It is very easy to break things while refactoring!

peddie commented 4 years ago

Ultimately, I'd like to handle arbitrary nesting (briefly discussed in #19 (comment)) but in the meantime manually adding additional instances will have to be the way to go.

Thanks -- that would be great. I saw that issue but it seemed like a much larger scope.

Could I ask you to add some test-cases for the instances you are adding? I'd normally add a type like your Foo above in test/TestBarbies.hs and test/TestBarbiesW.hs and declare all their instances. If you also declare the Show / Eq / Arbitrary instances, you can test the laws hold in test/Spec.hs. It is very easy to break things while refactoring!

No problem, and I should have provided these in the first place. I've updated this patch, along with a preceding (separate) commit adding a missing test case for the original NestedFW type.