UnkindPartition / traverse-with-class

MIT License
5 stars 5 forks source link

Adds GTraversable instance for Generic types #12

Closed ivanbakel closed 5 years ago

ivanbakel commented 5 years ago

The instance is for types where every field is significant (and therefore has the constraint hold). In this case, it's possible to write a traversal over all the fields using the conversion to/from the Generic representation.

The only questionable part of this design is the GTraversable' instance for composition of functors - there are other ways to write it, including a constraint of GTraversable' (GTraversable c) f, but since that makes no mention of g, it seemed incorrect.

UnkindPartition commented 5 years ago

Thanks, I think this mostly makes sense.

Like you, I'm sceptical of the :.: case. Intuitively, the constraint should be something like forall a . c (f (g a)) — and the implementation should simply unpack Comp1 and apply f.

You seem to have missed the Rec0 case, where the implementation should also just apply f I think.

Could you you bump the base constraint and adjust .travis.yml to account for the fact that this requires a newer version of the ghc? (And add a comment in the cabal file about why it's required.)

Some module-level documentation about how to use this / what it is for would also be nice.

ivanbakel commented 5 years ago

Rec0 should be covered by K1- is there a benefit to writing the special case? I'm not that familiar with the inner working of Generic.

The GHC bump seems to require at least 8.6 to make use of QuantifiedConstraints, which in retrospect is quite drastic. Should I go ahead with it, or lock the Rec1 instance behind a version gate with a compiler pragma?

UnkindPartition commented 5 years ago

Rec0 should be covered by K1- is there a benefit to writing the special case? I'm not that familiar with the inner working of Generic.

Ah, you're right, I didn't realize Rec0 was a synonym.

lock the Rec1 instance behind a version gate with a compiler pragma?

That sounds like a good idea — and please mention/explain that in the module docs.

UnkindPartition commented 5 years ago

Thanks, merged and uploaded as traverse-with-class-1.0.1.0.