Improving documentation

[Lev135]

I argue it would be very useful to reorganize lens haddock. I think it would be great if it will be rearranged in some uniform way. The great example of organization (in my opinion) is optics library. Of course, making huge lens documentation so well-structured as the tiny (in comparison with lens) optics package has is not very easy. However, I think we should try, since any step in this way is an essential improvement. I can do this work if the owners of this repo won't be too critical on these changes. I'd like to have a green light before doing the most of the work.

To be more concrete I've started the work on restructuring Setters. You can see the WIP situation here.
Also you can see it in a rendered html version here (and compare it with the current doc, abailable on Hackage).

[Lev135]

I've done the main part of the work on Setters. I think we should come to an agreement on (the most aspects of) the documentation structure and stylistics and then do others by the template. So I'll be glad to get any opinions about this.

[RyanGlScott]

Thanks for doing this, @Lev135! I've taken a brief look at the changes, and most of seems straightforward and uncontroversial.

The only part that I am wondering about are the parts of the patch that remove certain pieces of documentation for various things, such as ASetter. Can you explain more about your reasoning for this? (I don't necessarily think this is a bad thing, but it did surprise me when I saw it.)

[Lev135]

About ASetter etc.:

Running a 'Setter' instantiates it to a concrete type.

I don't understand what this sentence should clarify and even what does it mean

When consuming a setter directly to perform a mapping, you can use this type, but most
user code will not need to use this type.

It's not clear, what does "consume direcly to perform a mapping" meen.
I assume, that it's about consuming a setter to apply it not composing to other
lens, but it's not obvious: you can read this as "to perform a setter's
transormation (mapping)", that is totally wrong. And is this advise helpful at
all? Maybe just mentioning relationship between them (one is subtype of another
and one can use a clone... function to go vice versa) is more useful?

This is a useful alias for use when consuming a 'Setter''.
Most user code will never have to use this type.

These two sentence seems to contradict each other. If it's really useful, why
shouldn't I use it? Or it's useful only for library's internal? In this case,
I think, we shouldn't say it's useful, since we write documentation for library
user, not maintainer.

Summarising, maybe add something like the following to the section title,
without copy-pasting in all ASetter... type:

Rank-1 representation
This section containes rank-1 types, obtained by specializing functor type
in setters signature to Identity. In fact they are isomorphic to their
polymorphic counterparts: you can use a Setter as ASetter directly and
cloneSetter provides inverse convertion and similarly with other setters.
However, in most circumstances you don't need to use these types and functions
directly: you can just work with polymorphic setters and read in function
arguments ASetter as Setter, ASetter' as Setter' etc.

[Lev135]

Also I've removed the following notes to set and set':

Note: Attempting to 'set' a 'Fold' or 'Getter' will fail at compile time with an
relatively nice error message.

Since the type signature of set requires a setter and getters and folds are not a setter,
compilation error is expected without any extra notes, isn't it? What's "relatively nice
error message" is also unclear: at the first time I've seen this note to the set' function,
I've thought that it meens "set' provide better errors than set" but then I realised,
that exactly the same comment is provided to set function, and in relation to what
these messages are nice is a mistery to me (and I suppose, for other documentation
readers too)

[Lev135]

I'd like to ask you also, if my note to the <~ operator

Note that though it looks like a pass-through operator and pure ('.~'), ('%~')
operators it's unrelated to both of them.

is appropriate? Maybe I misunderstood something and they are indeed related (wrong doc is worse, than missed doc)

[RyanGlScott]

Regarding this:

Running a 'Setter' instantiates it to a concrete type.

My understanding is that several lens combinators use the A* family of types (e.g., ASetter) rather than their more general counterparts (e.g., Setter) to improve type inference. For example, ASetter has a more specific type than Setter:

type ASetter s t a b = (a -> Identity b) -> s -> Identity t
type Setter s t a b = forall f. Settable f => (a -> f b) -> s -> f t

There are various lens combinators which would typecheck regardless of whether you use ASetter or Setter as an argument, but using ASetter would improve error message since you are dealing with a concrete type Identity rather than a higher-rank type variable f. That leads into this sentence:

When consuming a setter directly to perform a mapping, you can use this type, but most
user code will not need to use this type.

When this says "most user code", I think it means that most code that constructs optics won't need to care about ASetter, since you'd want to construct a more general Setter. On the other hand, code that consumes optics (e.g., combinators that take optics as arguments) will likely want to use ASetter instead.

Admittedly, this is a tricky nuance to convey through documentation. Do you have a suggestion on how to communicate this information? Your suggested "Rank-1 representation" re-phrasing comes close, although I do think it's worth saying something about type inference and consuming-versus-producing there.

[RyanGlScott]

Note that though it looks like a pass-through operator and pure ('.'), ('%')
operators it's unrelated to both of them.

This isn't wrong necessarily, but I don't think it adds much, and I would prefer not including it. The fact is that most lens infix operators resemble each other, but that's mostly due to Haskell having very few character available for use in infix names in general. Moreover, there is a convention where you can attach a < character in front of an infix operator to obtain a version that returns a result (see this table), but <~ does not follow this convention (there is no ~ lens combinator). As such, I don't think it's worth drawing attention to this.

[Lev135]

Maybe we should add this:

There are various lens combinators which would typecheck regardless of whether you use ASetter or Setter as an argument, but using ASetter would improve error message since you are dealing with a concrete type Identity rather than a higher-rank type variable f.

and

most code that constructs optics won't need to care about ASetter, since you'd want to construct a more general Setter. On the other hand, code that consumes optics (e.g., combinators that take optics as arguments) will likely want to use ASetter instead.

straight-forward in the documentation to be more clear. Also we could add a note that it's can be useful to store setter in container. This can lead to a long enough comment, but here I think it's worth enough and it's not so long if we add this only in section heading. If you also think so, maybe you could rephrase your comments to me and maybe add something more based on your expirience and feeling this in the format of documentation? It definetly would be better than all I can do for it, because my expirience in lenses is far less, than yours.

[Lev135]

does not follow this convention

That's exactly what I wanted to point at. It seems also, based on your table, that there's a convention to use ~ in pure and = in state operators. And this operator doesn't follow both of them. That's why I tried to clarify it.

[Lev135]

Of course, when I said "looks like" I meant that it could be occidently considered to follow these conventions and not that it contains the same symbols. Also, I don't see more examples of this kind in the table

[RyanGlScott]

Maybe we should add this [...] and [...] straight-forward in the documentation to be more clear. Also we could add a note that it's can be useful to store setter in container.

I would support adding a top-level comment somewhere that describes the purpose of the A* family of type synonyms, and then we could include references to that comment from each A* type's Haddocks.

I generally like the phrasing that you chose, so feel free to go with that. It's possible that I will want to rephrase it slightly once I see it next to the surrounding documentation, but it's much easier to give suggestions of that sort in a GitHub PR review comment.

And this operator doesn't follow both of them. That's why I tried to clarify it.

Yeah, <~ is a weird one. It's intentionally meant to be a pun on the <- keyword used in Haskell's do-notation. (I think there is an example in the Haddocks for <~ which explains this.) In this sense, the "<" in <~ is completely unrelated to the "<" in most other infix lens operators. Perhaps it's worth a comment stating this—I'd be in favor of that.