Move the lenses generated by makeClassy into the class it generates #35

Closed
ekmett opened this Issue Sep 4, 2012 · 8 comments

2 participants

@ekmett
Owner

This would facilitate exporting the lenses in the export list because the enduser could just export HasFoo(..)

@mgsloan mgsloan was assigned Sep 4, 2012
@mgsloan
Collaborator

Hmm, I'm not so sure this makes sense. While at first this is what I thought makeClassy does, this might be verging dangerously on the territory where there might be actual properties relating the classy members ;)

Joking aside, is this a style to be encouraged? What's to say that a class-per-field isn't better?

@ekmett
Owner

I'd rather prefer to avoid the class per field design. the design of makeClassy enables you to get by with O(n) less work where n is the number of fields on average.

@mgsloan
Collaborator

Right, I don't think one class per field is the right choice.

One trickiness of this is something I mentioned before when I misunderstood Classy - it seems odd to have the incidental definitions of the data structure effect the type signatures of the methods. In other words, if a particular field happens to be a traversal, then the field in the class is going to need to be a traversal.

Maybe the solution is to force the user to write the typeclass, and only generate instances?

@ekmett
Owner

I'd rather avoid that as well. The nice thing about the makeClassy approach is that it transparently builds the class for you. If you look at most code that people were using back in the stone age with all the old lens libraries, they'd make a data type, make a class, generate lenses that just got the stuff out of their local data type, then they'd have to go through and do all the boilerplate of wiring it up to the class. The nice thing about makeClassy is that you only write

data Foo = Foo { _fooBar, _fooBaz :: Int }
makeClassy ''Foo

and it gives you the HasFoo, and the lenses for fooBar, and fooBaz that precompose the simple foo lens onto the front.

If you have to define the typeclass its pretty awful.

And if the field is a Traversal, then making it be a Traversal in the typeclass is pretty much exactly what you want, no?

@ekmett
Owner

This can only work if makeClassy builds the class.

There is a flag for whether or not it should.

Perhaps there should also be a flag that defaults to True to indicate if it should put it in the class.

@ekmett
Owner

Moved to 3.0.

@ekmett ekmett was assigned Sep 12, 2012
@ekmett
Owner

Just banged this out.

@ekmett ekmett added a commit that referenced this issue Sep 12, 2012
@ekmett makeClassy puts all exports in the class when it can (Issue #35). Add…
…ed -fdump-splices.
d85895c
@ekmett
Owner

Moved back to 2.9. =)

@ekmett ekmett closed this Sep 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment