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 #131

Merged
merged 12 commits into from Oct 24, 2013

Conversation

Projects
None yet
4 participants
@jeffreyrosenbluth
Member

jeffreyrosenbluth commented Oct 21, 2013

No description provided.

, cat, cat', CatOpts(..), CatMethod(..)
, cat, cat'
, CatOpts(..), catMethod, sep
, catOptsvProxy__ -- may not want to export?

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Yeah, let's not export this. There's no reason to. No one should have been using it before.

) where
import Control.Lens hiding ((#), (<.>), moveTo, at, beside)

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

How many things do we actually need from Control.Lens? In general I am a much bigger fan of explicit import lists than hiding clauses.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

It's no big deal, but we need more functions after we put the explicit type signatures in for the haddock docs.
In particular - makeLensesWith, lensRules, & generateSignatures

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Right. I'd still rather list them explicitly though. It means our code won't break if lens happens to add some new names in the future which clash with diagrams names (an entirely plausible scenario).

This comment has been minimized.

@jeffreyrosenbluth
-- please ignore it.
data CatOpts v = CatOpts { _catMethod :: CatMethod
, _sep :: Scalar v
, _catOptsvProxy__ :: Proxy v

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

The entire reason this catOptsvProxy__ thing existed was because record updates like with { sep = 3 } are potentially type-changing, so GHC did not know what type to infer for with. The solution was to include a dummy field to fix the type v. But now that the sep lens is explicitly NOT type-changing this problem goes away, so we can get rid of this silly hack of a field altogether!

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

Ah, another benefit of lenses arises. We should keep a list.

@@ -32,8 +27,8 @@ infixl 7 :&
-- | Types which are instances of the @Coordinates@ class can be
-- constructed using '&' (for example, a three-dimensional vector
-- could be constructed by @1 & 6 & 3@), and deconstructed using
-- constructed using '@@' (for example, a three-dimensional vector

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Even though I am the one who suggested it, I have to say that I am not a big fan of @@. I don't really have any better ideas though. =(

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

<joke> perhaps we can use &&&& (since &, &&, and &&& are all taken) </joke>

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

I agree @@ not great. See my not above about pr and mkR2
Are there any single characters left? I'm don't mind ><

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

More seriously: &. and .& were rightly rejected since there are too many operators from vector-space which use operators beginning or ending with periods (which are used to indicate a point argument). Sadly vector-space does not use any particular character to indicate scalar arguments, or we could steal it here. What about &^? Indicating that the first argument is a scalar and the second is a vector (well, or a point, but we can't win them all). It also has the important-in-practice-if-not-in-theory benefit of being very easy to type.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

I don't know about &^, it implies an asymmetry, but the most common use case I have seen has both arguments as scalars.

This comment has been minimized.

@fryguybob

fryguybob Oct 21, 2013

Member

I do use & and would like something nice to replace it, >< sort of looks like an X for times and that makes sense, but it is used in Data.Seq and QuickCheck. I'm not too bothered by the asymmetry of &^ or ^&.

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Oh, you're right, I was confused about the fixity. ^& would work indeed. Ryan is right about >< being used in a few standard libraries. Unless there are any further objections, let's go with ^&. I hope this can be accomplished by some quick global search-and-replace scripts---I don't think there's anywhere else that @@ would show up.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

Would this do it?

perl -pi -e 's/@@/^&/g' /mypath/diagrams-lib/src/diagrams/*

and also on TwoD, ThreeD and the other subdirectories

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

That's still too much work (you have to repeat it for every subdirectory of
every repository). Using zsh I would do something like

perl -pi -e 's/@@/^&/g' /mypath/**/*.{hs,rst}

i.e. do the replacement on every .hs and .rst file occurring anywhere in
the directory hierarchy. Bash globbing is not quite as powerful, but you
can instead do something like

find . -name '*.hs' -or -name '*.rst' | xargs -L 1 perl -e 's/@@/^&/g'

and then of course carefully examine the diff before recording a git commit
in each repo, to make sure it did the right thing.

On Mon, Oct 21, 2013 at 8:13 AM, Jeffrey Rosenbluth <
notifications@github.com> wrote:

In src/Diagrams/Coordinates.hs:

@@ -32,8 +27,8 @@ infixl 7 :&

-- | Types which are instances of the @coordinates@ class can be
--- constructed using '&' (for example, a three-dimensional vector
--- could be constructed by @1 & 6 & 3@), and deconstructed using
+-- constructed using '@@' (for example, a three-dimensional vector

Would this do it?

perl -pi -e 's/@@/^&/g' /mypath/diagrams-lib/src/diagrams/*

and also on TwoD, ThreeD and the other subdirectories


Reply to this email directly or view it on GitHubhttps://github.com//pull/131/files#r7091615
.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

I realized my editor Sublime Text 2 can handle it in one a couple of mouse clicks.
Already done & checked

-- @
--
-- Note that @&@ is left-associative.
(&) :: PrevDim c -> FinalCoord c -> c
-- Note that @\@\^& is left-associative.

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Syntax needs to be fixed here

-- Note that @\@\^& is left-associative.
(^&) :: PrevDim c -> FinalCoord c -> c
-- | Prefix synonym for @\@\^&. pr stands for pair of @PrevDim@, @FinalCoord@

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

syntax

@@ -12,10 +13,14 @@
-----------------------------------------------------------------------------
module Diagrams.Parametric.Adjust
( adjust
, AdjustOpts(..), AdjustMethod(..), AdjustSide(..)
, AdjustOpts(..)
, adjMethod, adjSide, adjEps, adjOptsvProxy__

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

We don't want to export adjOptsvProxy__.

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Note, however, that we can't get rid of the field entirely unless we explicitly give the adjMethod and adjEps lenses more restricted types (making then Lens' instead of Lens). (Perhaps that is a good idea.)

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

makeLenses does this already e.g.

adjEps :: forall v. Lens' (AdjustOpts v) (Scalar v)

so can i get rid of proxy?

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Ah, that makes sense. Indeed.

, ArcLength(..), getArcLengthCached, getArcLengthFun, getArcLengthBounded
, TotalOffset(..)
, OffsetEnvelope(..)
, SegCount(..), getSegCount

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Hmm, so we have a similar thing going on here with accessors that were named getFoo which are now lenses, which seems strange. I would be inclined to just name them foo instead. I think those names are currently in use for something else (I forget what), but not in a very consistent way, so I would be inclined to rename those other things and let the unadorned name be for the lenses.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

I assume you are just referring to SegCount all of the ArcLength functions come from Parametric

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

I am referring to getSegCount, getArcLength, getTotalOffset, and any other field labels named get.... getArcLengthCached and so on are fine since they don't correspond to field labels.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

Even though arcLength is a function in the Parametric class

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Hmmm... you're right. I don't know. Naming things is hard.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

I'm going to hate myself for suggesting this but perhaps lenses should end in L.
arcLengthL or maybe in cases like this where we have lots of functions with arcLength as part of the name, arcLengthLens

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

btw, speaking strictly mathematically a straight line is a curve.

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

I thought of an L suffix too, but I feel like if we are going to do that we should do it everywhere, which is definitely overkill. Yes, I know that mathematically a straight line is a curve, I think my real problem with it is just consistency. Having the lens for ArcLength called curveLength is just strange, no one would ever remember it.

Perhaps we should just stick with getArcLength.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 21, 2013

Member

Yeah, let's stick with getArcLength for now. When we are all done, we can see if this issue crops up enough to have some general strategy for naming lenses. If it's just a one-off it's no big deal.

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

OK, sounds good. Fortunately, not many end users will ever need this particular lens.

@@ -147,9 +148,12 @@ instance ( HasLinearMap (V a), InnerSpace (V a), OrderedField (Scalar (V a))
-- (/e.g./, split off the smallest number of segments from the
-- beginning which have a combined arc length of at least 5).
newtype SegTree v = SegTree
{ getSegTree :: FingerTree (SegMeasure v) (Segment Closed v) }
{ _getSegTree :: FingerTree (SegMeasure v) (Segment Closed v) }
deriving (Eq, Ord, Show)

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Same thing goes for the name getSegTree --- let's just call it segTree (and rename whatever currently has that name, if anything).

-- | Compute the total offset of anything measured by 'SegMeasure'.
offset :: ( Floating (Scalar v), Ord (Scalar v), InnerSpace v,
FT.Measured (SegMeasure v) t
)
=> t -> v
offset = trailMeasure zeroV (getTotalOffset . oeOffset)
offset = trailMeasure zeroV (view getTotalOffset . view oeOffset)

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Might this read better as (view (oeOffset.getTotalOffset))?

This comment has been minimized.

@jeffreyrosenbluth
@@ -792,12 +796,13 @@ trailFromVertices = wrapTrail . lineFromVertices
--
-- <<diagrams/src_Diagrams_Trail_glueLineEx.svg#diagram=glueLineEx&width=500>>
--
-- > import Diagrams.Coordinates
-- > glueLineEx = pad 1.1 . hcat' with {sep = 1}
-- > import Control.Lens ((&), (.~))

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

This Control.Lens import shouldn't be necessary if we are re-exporting those lens operators from Diagrams.Prelude.

@@ -193,7 +193,9 @@ arcBetween p q ht = trailLike (a # rotateBy (direction v) # moveTo p)
--
-- <<diagrams/src_Diagrams_TwoD_Arc_annularWedgeEx.svg#diagram=annularWedgeEx&width=400>>
--
-- > annularWedgeEx = hcat' with {sep = 0.50}
-- > import Control.Lens ((.~), (&))

This comment has been minimized.

@byorgey

byorgey Oct 21, 2013

Member

Again, the Control.Lens import should be unnecessary. I won't comment on any more but we should remove all of them.

the arrow function and magically have the joint fill color update to
match. The downside is that the *only* way to change the color of an
arrow shaft is to modify shaftStyle. Well, tough luck.
-- | Style to apply to the head.

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

Maybe we should mention something in the docs for headStyle, tailStyle, and shaftStyle about applying functions like fc red to them with %~.

'fc' and force the user to override it in the ArrowOpts, just as
with line color for the shaft.
-}
-- | Append the default shaft style to the left of the default shaftStyle

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

Is there a better way to word this comment? "Wibble the default foo style on the default fooStyle" is confusing.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 22, 2013

Member

These are not exported so I should just make them regular comments. But I will change the wording

@@ -56,20 +68,11 @@ showOrigin' :: (Renderable (Path R2) b, Backend b R2, Monoid' m)
=> OriginOpts -> QDiagram b R2 m -> QDiagram b R2 m
showOrigin' oo d = o <> d
where o = stroke (circle sz)
# fc (oColor oo)
# fc (oo^.oColor)

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

ooo, color!

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 22, 2013

Member

aa^.hOpacity !

@@ -333,8 +338,11 @@ trailCrossings p@(unp2 -> (x,y)) tr
-- concatenation, so applying multiple clipping paths is sensible.
-- The clipping region is the intersection of all the applied
-- clipping paths.
newtype Clip = Clip { getClip :: [Path R2] }
newtype Clip = Clip { _getClip :: [Path R2] }

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

Rename to clip if that's not already taken?

{ unScaleInv :: t
, scaleInvDir :: R2
, scaleInvLoc :: P2
{ _unScaleInv :: t

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

We should rename unScaleInv too. Perhaps scaleInvObj?

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

And maybe re-export unScaleInv = view scaleInvObj.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 22, 2013

Member

I'm guessing that we can get away without the re-export. If someone complains we can add it.

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

I was referring more just to exporting it from this module, to be used elsewhere in diagrams-lib (is it?). I agree we don't need to export it from Prelude. I don't think it matters too much either way.

This comment has been minimized.

@jeffreyrosenbluth

jeffreyrosenbluth Oct 22, 2013

Member

It's not currently re-exported. I don't think there is much code out there that will break. But I can re-export it if you prefer. I think it's easier to remove now, than to deprecate later.

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

OK, don't worry about re-exporting it then.

@byorgey

This comment has been minimized.

Member

byorgey commented Oct 22, 2013

OK, I think I've finished looking over all the changes in this pull request. If I didn't comment on something you can assume it's because I thought it looked good.

-- | Style to apply to the head. @headStyle@ is modified by using the lens
-- combinator @%~@ to change the current style. For example, to change
-- an opaque black arrowhead to translucent orange:
-- @(with & fc orange . opacity 0.75)@.

This comment has been minimized.

@byorgey

byorgey Oct 22, 2013

Member

This comment needs to be fixed, should say with & headStyle %~ fc orange . opacity 0.75.

This comment has been minimized.

@jeffreyrosenbluth
@jeffreyrosenbluth

This comment has been minimized.

Member

jeffreyrosenbluth commented on eefb797 Oct 23, 2013

OK, I think that lens is be ready to be merged.

Brent Yorgey added some commits Oct 24, 2013

Brent Yorgey
reinstate pathTrails as projection function
For backwards compatibility---it's something users could reasonably
have been using, and it has a nice name that's unlikely to clash with
anything else we want to do.  Power users who want the iso can use the
new Wrapped instance.
Brent Yorgey
Revert "removed catOptsProxy"
The reason for removing the catOptsProxy was that the new CatOpts
lenses have restricted types, so we don't run into the problems with
type inference that you get when doing record updates.  However, for
this release at least, we want to offer users the upgrade path of
simply sticking underscores on their record labels and continuing to
do record updates, so we still need the proxy, otherwise users' code
will break.

This reverts commit caf79c8.

Brent Yorgey added some commits Oct 24, 2013

@byorgey byorgey merged commit e6b17be 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