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

API documentation for `Prism` and `Traversal` #89

Merged
merged 6 commits into from May 15, 2018

Conversation

Projects
None yet
2 participants
@marick
Copy link
Collaborator

marick commented May 7, 2018

This provides top-of-page summary documentation for Prism and Traversal. It also contains documentation for many Prism and Traversal functions (not all).

@@ -1,7 +1,46 @@
-- | This module defines functions for working with prisms.
-- | Prisms are used for selecting cases of a type, most often a sum

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

Law abiding prisms only apply to types which are isomorphic to sums. I wonder if that could be brought out here. Maybe it's not that important though.

This comment has been minimized.

@marick

marick May 9, 2018

Collaborator

Generally, I'd prefer to have all laws explained in one file, because the law differences between different optics are instructive.

In this particular case, I envision that the sum-type-specific examples file will be joined with a file about using prisms on things isomorphic to sum types.

@@ -18,23 +57,78 @@ import Data.Profunctor (dimap, rmap)
import Data.Profunctor.Choice (right)
import Data.Newtype (under)

-- | Create a `Prism` from a constructor/pattern pair.
-- | Create a `Prism` from a constructor and a "focus" function that

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

Might be worth commenting on the Either t bit here? Specifically, it wasn't obvious to me why there was a t there at all.

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

Also, it might be worth talking about the prism laws briefly here?

This comment has been minimized.

@marick

marick May 9, 2018

Collaborator

I don't understand why it's Either t a instead of Either somethingElse a, so I can't explain it.

Regarding laws: I do cover laws in Lenses for the Mere Mortal, in more detail than can be done in an API document. Provided I continue to make it free, that seems a better place to talk about laws.

This comment has been minimized.

@paf31

paf31 May 10, 2018

Member

I think you could explain it succinctly as

_Note_: The matching function returns a result wrapped in `Either t` so as to allow for type-changing prisms in the case where the input does not match.

What do you think?

This comment has been minimized.

@paf31

paf31 May 10, 2018

Member

I disagree about the laws actually. I think they ought to be documented with the types themselves, like we do for type classes in Prelude.

This comment has been minimized.

@paf31

paf31 May 10, 2018

Member

This isn't to say we need to add them here now, just that I'd like to eventually, and that redirecting readers to an external resource from doc comments doesn't strike me as ideal.

This comment has been minimized.

@marick

marick May 10, 2018

Collaborator

I do refer to the laws section of the Haskell lens fairly often, so I take your point.

I tend to explain laws in words, then equational and example style:

set-get:

: `view` retrieves what `set` puts in. 
  
  {lang=haskell}
  ~~~~~~~~~~~~~~
  set lens >>> view lens ≡ id
  
  > set first "NEW" (Tuple 1 2) # view first
  "NEW"
  ~~~~~~~~~~~~~~

That OK?

This comment has been minimized.

@paf31

paf31 May 10, 2018

Member

Yeah I like that.

This comment has been minimized.

@marick

marick May 11, 2018

Collaborator

How do I explain the Traversal laws? The Haskell traversal laws don't translate into profunctor lenses, and it seems odd to write:

traverseOf traversed pure ≡ pure

This comment has been minimized.

@marick

marick May 11, 2018

Collaborator
over traversed pure ≡ map pure

?

prism :: forall s t a b. (b -> t) -> (s -> Either t a) -> Prism s t a b
prism to fro pab = dimap fro (either id id) (right (rmap to pab))

-- | Create a `Prism` from a constructor and a "focus" function that

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

What do you think about calling this a matching function? I compare it to pattern matching on the constructor.

This comment has been minimized.

@marick

marick May 9, 2018

Collaborator

I'll do that.

-- | Review a value through a `Prism`.
review :: forall s t a b. Review s t a b -> b -> t
review = under Tagged
-- | Create a prism that focuses on only some of the values of a case,

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

In order for this to work as a law-abiding prism, the predicate should identify only the value passed it. It's identifying the one value as a summand of the whole.

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

For example, lens in Haskell uses nearly [] null as an example. Note that [] is the only value for which null evaluates to true.

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

Actually, this also applies to only, since Eq isn't necessarily going to give definitional equality. That might be worth commenting on briefly as well.

This comment has been minimized.

@marick

marick May 10, 2018

Collaborator

I don't understand "identify only the value passed it".

This comment has been minimized.

@marick

marick May 10, 2018

Collaborator

I'll make explicit only's dependence on Eq.

This comment has been minimized.

@marick

marick May 10, 2018

Collaborator

What would be a use of nearly that would make an ill-behaved prism?

This comment has been minimized.

@paf31

paf31 May 10, 2018

Member

I don't understand "identify only the value passed it".

I mean that f a can only hold when a = x and not otherwise.

I'll make explicit only's dependence on Eq.

It's not just the dependence on Eq, but that that instance must be definitional equality: eq x y if and only if x = y definitionally.

What would be a use of nearly that would make an ill-behaved prism?

Your example gives a prism which does not obey the second prism law. Extracted from the lens docs:

If preview l s ≡ Just a then review l a ≡ s

i.e. if predicate s then const (Solid referenceColor) = s. This obviously only holds for one value of s, not all of them.

This comment has been minimized.

@marick

marick May 10, 2018

Collaborator

I completely misunderstood the purpose of nearly if there's only one possible s that can satisfy the predicate. Is it then the case that the difference between nearly and only just that the latter uses Eq whereas the former uses the given predicate to detect the one-and-only-one value that's allowed?

This comment has been minimized.

@paf31

paf31 May 10, 2018

Member

That's right. Per the lens docs:

To comply with the Prism laws the arguments you supply to nearly a p are somewhat constrained.

We assume p x holds iff x ≡ a. Under that assumption then this is a valid Prism.

@@ -1,12 +1,30 @@
-- | This module defines functions for working with traversals.
-- | `Traversal` is an optic that focuses on zero or more functor values. An

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

Not necessarily a Functor, but something isomorphic to a Traversable functor (just a Functor gives a Setter actually). For example, I can traverse the chars in a string.

This comment has been minimized.

@marick

marick May 10, 2018

Collaborator

I'll just say "Traversable functor". My goal here is to allow people to use Traversal without having to first know Traversable (not in all cases, but the simple ones).

This comment has been minimized.

@paf31

paf31 May 10, 2018

Member

The String/Char example does not involve a Traversable functor though. I think simply removing the word functor would be enough.

@@ -56,7 +102,14 @@ failover t f s = case unwrap (t $ Star $ Tuple (Disj true) <<< f) s of
Tuple (Disj true) x -> pure x
Tuple (Disj false) _ -> empty

-- | Affine traversal the `n`-th focus of a `Traversal`.
-- | Combine an index and a traversal to narrow the focus to a single

This comment has been minimized.

@paf31

paf31 May 7, 2018

Member

Maybe could add something like

This is called an "affine traversal", _which means that the result focuses on one or zero (if the index is out of range) results_.

This comment has been minimized.

@marick

marick May 10, 2018

Collaborator

Will do.

@paf31

This comment has been minimized.

Copy link
Member

paf31 commented May 7, 2018

Thanks! Looks really good, most of my comments are nitpicks, with the one exception of the comment on nearly where I think we need to be a bit more precise about the laws.

@marick

This comment has been minimized.

Copy link
Collaborator

marick commented May 12, 2018

I believe I've addressed all the earlier comments.

The only thing that's missing is Traversal laws, because I don't understand them.

@paf31

paf31 approved these changes May 14, 2018

Copy link
Member

paf31 left a comment

Thanks very much! Looks great, please feel free to merge.

@marick marick merged commit 2d10428 into purescript-contrib:master May 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment