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

Lens #44

Merged
merged 16 commits into from Oct 24, 2013

Conversation

Projects
None yet
4 participants
@jeffreyrosenbluth
Member

jeffreyrosenbluth commented Oct 21, 2013

No description provided.

@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Oct 7, 2013

Member

Can you use runQuery :: Iso' (Query v m) (Point v -> m) ?

Member

jeffreyrosenbluth commented on src/Diagrams/Core/Query.hs in cc00945 Oct 7, 2013

Can you use runQuery :: Iso' (Query v m) (Point v -> m) ?

This comment has been minimized.

Show comment
Hide comment
@bergey

bergey Oct 7, 2013

Member

I gave runQuery a slightly more general type --- the m type can change. I'm not sure if this is right or useful, though. It could generalize more, with v' next to m' When I was writing the Iso for QDiagram, generalizing the m type was necessary for the obvious definition of the Functor instance.

Member

bergey replied Oct 7, 2013

I gave runQuery a slightly more general type --- the m type can change. I'm not sure if this is right or useful, though. It could generalize more, with v' next to m' When I was writing the Iso for QDiagram, generalizing the m type was necessary for the obvious definition of the Functor instance.

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Oct 7, 2013

Member

I wrote an explicit Iso for pathTrails out of a slight desire to avoid using Template Haskell. However, now that we are going all-out for lenses, avoiding Template Haskell entirely would be way too much work for not much benefit. Therefore we might as well just use makeLenses here for runQuery (which will generate the most general possible Iso).

Member

byorgey replied Oct 7, 2013

I wrote an explicit Iso for pathTrails out of a slight desire to avoid using Template Haskell. However, now that we are going all-out for lenses, avoiding Template Haskell entirely would be way too much work for not much benefit. Therefore we might as well just use makeLenses here for runQuery (which will generate the most general possible Iso).

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Oct 7, 2013

Member

@byorgey I went back last night and replaced some uses of makeLenses with iso and I know @bergey uses iso extensively. Are you suggesting we change this to us makeLenses consistently throughout and let Template Haskell do it's thing ?

Member

jeffreyrosenbluth replied Oct 7, 2013

@byorgey I went back last night and replaced some uses of makeLenses with iso and I know @bergey uses iso extensively. Are you suggesting we change this to us makeLenses consistently throughout and let Template Haskell do it's thing ?

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Oct 7, 2013

Member

Yes. Template Haskell is generating exactly that code with iso so there's no need to write it ourselves. This way is also better insurance against future changes to the lens library and/or our definitions.

Member

byorgey replied Oct 7, 2013

Yes. Template Haskell is generating exactly that code with iso so there's no need to write it ourselves. This way is also better insurance against future changes to the lens library and/or our definitions.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Oct 21, 2013

Member

Hmm, there are quite a few things in -core which were cute as accessor names but are sort of strange and ugly as lens names, e.g. unTransInv, appTrace, inEnvelope, runQuery, unQD, unSubMap. Basically anything beginning with app, run, un... Since we're going to be breaking things anyway I wonder if we should consider renaming these. Actually, in the case of appFoo and runQuery we could even leave appFoo and runQuery as backwards-compatible partial applications of view. What do you think?

As concrete suggestions, perhaps something like traceFun, envelopeFun, and queryFun? (or ...Func, or ...F?) And instead of unQD we can just have qd, and so on.

Thoughts?

Member

byorgey commented Oct 21, 2013

Hmm, there are quite a few things in -core which were cute as accessor names but are sort of strange and ugly as lens names, e.g. unTransInv, appTrace, inEnvelope, runQuery, unQD, unSubMap. Basically anything beginning with app, run, un... Since we're going to be breaking things anyway I wonder if we should consider renaming these. Actually, in the case of appFoo and runQuery we could even leave appFoo and runQuery as backwards-compatible partial applications of view. What do you think?

As concrete suggestions, perhaps something like traceFun, envelopeFun, and queryFun? (or ...Func, or ...F?) And instead of unQD we can just have qd, and so on.

Thoughts?

@bergey

This comment has been minimized.

Show comment
Hide comment
@bergey

bergey Oct 21, 2013

Member

Yeah, I noticed those. The unFoo series especially bother me. I didn't want to come up with a new scheme myself, especially since I was changing code that I had never looked at before. Any of your suggestions seem better than the current naming. I don't like fooFun as much - every time I see names like that I stop to wonder what's so fun about foo.

Member

bergey commented Oct 21, 2013

Yeah, I noticed those. The unFoo series especially bother me. I didn't want to come up with a new scheme myself, especially since I was changing code that I had never looked at before. Any of your suggestions seem better than the current naming. I don't like fooFun as much - every time I see names like that I stop to wonder what's so fun about foo.

@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

I agree, we should change the names and I don't like fooFun either. I have a slight preference for FooF

Member

jeffreyrosenbluth commented Oct 21, 2013

I agree, we should change the names and I don't like fooFun either. I have a slight preference for FooF

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Oct 21, 2013

Member

OK, point taken, forget about fooFun then. Shall we just go with fooF then? (Of course we can't generally use foo because those names are already taken --- envelope, trace, query, etc.)

Concrete suggestions:

  • unTransInv -> transInv
  • appTrace -> traceF (and re-export appTrace = view traceF)
  • inEnvelope -> envelopeF
  • runQuery -> queryF (and re-export runQuery = view queryF)
  • unQD -> qd
  • unSubMap -> ... hmm, not sure about this one. subMap is already taken.
Member

byorgey commented Oct 21, 2013

OK, point taken, forget about fooFun then. Shall we just go with fooF then? (Of course we can't generally use foo because those names are already taken --- envelope, trace, query, etc.)

Concrete suggestions:

  • unTransInv -> transInv
  • appTrace -> traceF (and re-export appTrace = view traceF)
  • inEnvelope -> envelopeF
  • runQuery -> queryF (and re-export runQuery = view queryF)
  • unQD -> qd
  • unSubMap -> ... hmm, not sure about this one. subMap is already taken.
@bergey

This comment has been minimized.

Show comment
Hide comment
@bergey

bergey Oct 23, 2013

Member

I restored appTrace and runQuery as non-lens field accessors, without an underscore. I removed the other newtype field names while adding Wrapped instances. I'm fine putting them back, of course.

I used a mix of makeWrapped calls (where easy) and explicit instances. The explicit instances help in Types.hs, because of the cyclic dependencies, and where there are complicated type constraints, in particular where the type needs to refer to a type family.

makeWrapped produces some very ugly signatures:
Wrapped (Point v0 -> m0) (Point v_16274429970 -> m_16274429980) (Query v0 m0) (Query v_16274429970 m_16274429980)
One last thing to decide is whether it's worth replacing makeWrapped with explicit instances to have nicer signatures. I don't see an equivalent to makeLensesWith.

Member

bergey commented Oct 23, 2013

I restored appTrace and runQuery as non-lens field accessors, without an underscore. I removed the other newtype field names while adding Wrapped instances. I'm fine putting them back, of course.

I used a mix of makeWrapped calls (where easy) and explicit instances. The explicit instances help in Types.hs, because of the cyclic dependencies, and where there are complicated type constraints, in particular where the type needs to refer to a type family.

makeWrapped produces some very ugly signatures:
Wrapped (Point v0 -> m0) (Point v_16274429970 -> m_16274429980) (Query v0 m0) (Query v_16274429970 m_16274429980)
One last thing to decide is whether it's worth replacing makeWrapped with explicit instances to have nicer signatures. I don't see an equivalent to makeLensesWith.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Oct 23, 2013

Member

Ugh, those are really ugly signatures indeed. How much work is it to write the signatures manually? Or, if we want to be extra ambitious, how hard would it be to submit a patch for makeWrapped which generates nicer signatures? @mgsloan, maybe you have an idea about that?

Member

byorgey commented Oct 23, 2013

Ugh, those are really ugly signatures indeed. How much work is it to write the signatures manually? Or, if we want to be extra ambitious, how hard would it be to submit a patch for makeWrapped which generates nicer signatures? @mgsloan, maybe you have an idea about that?

@mgsloan

This comment has been minimized.

Show comment
Hide comment
@mgsloan

mgsloan Oct 23, 2013

Member

@byorgey Yeah, certainly seems doable - it should be a matter of replacing a newName with a mkName. Since this is a top-level definition, there's really no reason to have unique names for its type variables!

You would have to make sure that none of the type variable names you use alias eachother, though. This is probably why it uses newName.

Member

mgsloan commented Oct 23, 2013

@byorgey Yeah, certainly seems doable - it should be a matter of replacing a newName with a mkName. Since this is a top-level definition, there's really no reason to have unique names for its type variables!

You would have to make sure that none of the type variable names you use alias eachother, though. This is probably why it uses newName.

write Wrapped instances manually
makeWrapped generates type variables with ugly large numbers
@bergey

This comment has been minimized.

Show comment
Hide comment
@bergey

bergey Oct 23, 2013

Member

Writing the instances manually is very easy. I'd rather leave improving makeWrapped to someone who's used TH before.

I think this branch is just waiting for -lib to be ready, now. (So let me know if you see something I missed.)

Member

bergey commented Oct 23, 2013

Writing the instances manually is very easy. I'd rather leave improving makeWrapped to someone who's used TH before.

I think this branch is just waiting for -lib to be ready, now. (So let me know if you see something I missed.)

@mgsloan

This comment has been minimized.

Show comment
Hide comment
@mgsloan

mgsloan Oct 23, 2013

Member

Yeah, that's quite reasonable. Having TH handle the boilerplate of isos is much less important than with lenses / traversals / etc.

Member

mgsloan commented Oct 23, 2013

Yeah, that's quite reasonable. Having TH handle the boilerplate of isos is much less important than with lenses / traversals / etc.

@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Oct 23, 2013

Member

Sounds like the way to go, I'll do the same for the -lib, etc.

Member

jeffreyrosenbluth commented on b0e9f1f Oct 23, 2013

Sounds like the way to go, I'll do the same for the -lib, etc.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Oct 23, 2013

Member

OK, sounds good. And re: merging, yeah, once we polish up all the lens branches I'll merge them all at once.

Member

byorgey commented Oct 23, 2013

OK, sounds good. And re: merging, yeah, once we polish up all the lens branches I'll merge them all at once.

byorgey added a commit that referenced this pull request Oct 24, 2013

@byorgey byorgey merged commit fff5211 into master Oct 24, 2013

1 check passed

default The Travis CI build passed
Details

@byorgey byorgey deleted the lens branch Oct 24, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment