Improved documentation of Signal.foldp

[jvoigtlaender]

@mietek asked what happened to this commit. It's indeed a bit mysterious. @evancz had merged the PR with that commit (see https://github.com/elm-lang/core/pull/149), but somehow it went missing from the repository's history.

So I've repeated the change in this PR here, and hope it can be quickly merged.

[jvoigtlaender]

@evancz, here's another instance of how the deficiency of the current docs in this respect is hurting: latest thread on the mailing list. That person shouldn't have to ask on the mailing list

Where does the initial signalled value get lost?

Instead, the documentation of foldp should have told them. That's what the note added here does. Also, you had already approved the note and its wording when you merged pull request https://github.com/elm-lang/core/pull/149 almost a year ago.

[jvoigtlaender]

@evancz, here's another instance of how the deficiency of the current docs in this respect is hurting: latest thread on the mailing list. That person shouldn't have to ask on the mailing list

Where does the initial signalled value get lost?

Instead, the documentation of foldp should have told them. That's what the note added here does. Also, you had already approved the note and its wording when you merged pull request https://github.com/elm-lang/core/pull/149 almost a year ago.

[mietek]

@jvoigtlaender, I must note that the note added here does not explain why the initial value of the signal is ignored. I still don’t understand the reason.

[mietek]

@jvoigtlaender, I must note that the note added here does not explain why the initial value of the signal is ignored. I still don’t understand the reason.

[mietek]

I also don’t understand how to use foldp' to work around this problem. Here’s what I did instead, in order to continue using StartApp.

type Model = ...

initialModel : Model
initialModel = ...

type Action = Idle | ...

init : (Model, Effects Action)
init =
    (initialModel, Effects.task (Task.succeed Idle))

update : Action -> Model -> (Model, Effects Action)
update action model =
    case action of
      Idle ->
        (model, Effects.none)
      ...

[mietek]

I also don’t understand how to use foldp' to work around this problem. Here’s what I did instead, in order to continue using StartApp.

type Model = ...

initialModel : Model
initialModel = ...

type Action = Idle | ...

init : (Model, Effects Action)
init =
    (initialModel, Effects.task (Task.succeed Idle))

update : Action -> Model -> (Model, Effects Action)
update action model =
    case action of
      Idle ->
        (model, Effects.none)
      ...

[jvoigtlaender]

@mietek, yes, the documentation with my added note only says what the behavior of foldp is, not why it is that way. I don't think the documentation of a function should say why it is not another function? For example, when reading the documentation of List.filter, do you expect a justification for the fact that the function keeps, rather than drops, the elements that satisfy the given predicate? The other version (dropping rather than keeping) would also have been an option. But the choice was made to implement the version that keeps. The documentation should state what is the case, and it does. The same should apply to the documentation of foldp, I think.

Separately from the issue of documentation, I wouldn't know what to say (here or elsewhere) about "why" foldp is as it is. Just as for filter, it was a choice. To either include in core the version that we now know as foldp, or the version we now know as foldp'. Evan made that choice, and it has been like this since then.

About how to use foldp' to work around this problem: Does this help? It's not in the context of start-app, but that's simply because in start-app the foldp is hidden from the programmer, so it cannot be replaced by foldp'. If you want a solution in the context of start-app, you need to wait for the start-app package to be revised, or use a fork of it. See evancz/start-app#37.

[jvoigtlaender]

@mietek, yes, the documentation with my added note only says what the behavior of foldp is, not why it is that way. I don't think the documentation of a function should say why it is not another function? For example, when reading the documentation of List.filter, do you expect a justification for the fact that the function keeps, rather than drops, the elements that satisfy the given predicate? The other version (dropping rather than keeping) would also have been an option. But the choice was made to implement the version that keeps. The documentation should state what is the case, and it does. The same should apply to the documentation of foldp, I think.

Separately from the issue of documentation, I wouldn't know what to say (here or elsewhere) about "why" foldp is as it is. Just as for filter, it was a choice. To either include in core the version that we now know as foldp, or the version we now know as foldp'. Evan made that choice, and it has been like this since then.

About how to use foldp' to work around this problem: Does this help? It's not in the context of start-app, but that's simply because in start-app the foldp is hidden from the programmer, so it cannot be replaced by foldp'. If you want a solution in the context of start-app, you need to wait for the start-app package to be revised, or use a fork of it. See evancz/start-app#37.

[mietek]

@jvoigtlaender, the documentation describes foldp as “Create a past-dependent signal”, and yet the result does not depend on the incoming signal’s initial value, which is most certainly in the past. To my mind, the description is borderline incoherent, and the behaviour is an unpleasant surprise.

The List.filter analogy is unsatisfactory. Given that filter means “keeping” in other languages, I would expect any new language that chooses filter to mean “dropping” to justify the choice in the documentation. Additionally, “keeping” and “dropping” are the only two unsurprising behaviours for filter. I would not expect filter to keep all the elements of a list that satisfy a given predicate, except the first.

[mietek]

@jvoigtlaender, the documentation describes foldp as “Create a past-dependent signal”, and yet the result does not depend on the incoming signal’s initial value, which is most certainly in the past. To my mind, the description is borderline incoherent, and the behaviour is an unpleasant surprise.

The List.filter analogy is unsatisfactory. Given that filter means “keeping” in other languages, I would expect any new language that chooses filter to mean “dropping” to justify the choice in the documentation. Additionally, “keeping” and “dropping” are the only two unsurprising behaviours for filter. I would not expect filter to keep all the elements of a list that satisfy a given predicate, except the first.

[jvoigtlaender]

Okay, maybe the filter analogy was a stretch. Still, I'm not exactly sure what you want to be added to the documentation of foldp in addition to what I already add here. You say:

To my mind, the description is borderline incoherent, and the behaviour is an unpleasant surprise.

With my added text, the behavior would not anymore be a surprise. It might still be unpleasant behavior, but that means you are calling for the function to be replaced by a different one, not for a change to the documentation. Also "Create a past-dependent signal" plus "but the initial value is not used" explains exactly what the function does, so I don't know what is wrong about that in combination.

And as for providing justification in the documentation, I have already remarked that I'm not the person to give that justification. And I doubt Evan wants to give it. So none of this should stop this pull request from being merged. It strictly improves the current documentation, and I don't see a further strict improvement being proposed.

[jvoigtlaender]

Okay, maybe the filter analogy was a stretch. Still, I'm not exactly sure what you want to be added to the documentation of foldp in addition to what I already add here. You say:

To my mind, the description is borderline incoherent, and the behaviour is an unpleasant surprise.

With my added text, the behavior would not anymore be a surprise. It might still be unpleasant behavior, but that means you are calling for the function to be replaced by a different one, not for a change to the documentation. Also "Create a past-dependent signal" plus "but the initial value is not used" explains exactly what the function does, so I don't know what is wrong about that in combination.

And as for providing justification in the documentation, I have already remarked that I'm not the person to give that justification. And I doubt Evan wants to give it. So none of this should stop this pull request from being merged. It strictly improves the current documentation, and I don't see a further strict improvement being proposed.

[jvoigtlaender]

I guess what I'm trying to say is that there are separate issues coming up in the above discussion:

1. foldp behaves somewhat strangely
2. the current documentation of foldp doesn't mention that strangeness
3. the current documentation of foldp doesn't justify that strangeness

All I can do is address 2. here. If 1. and/or 3. should be addressed, that should be brought up on the mailing list or in separate issues in this repository.

If there is some way to address 2. better than I have in the commit above, I'm all ears.

[jvoigtlaender]

I guess what I'm trying to say is that there are separate issues coming up in the above discussion:

1. foldp behaves somewhat strangely
2. the current documentation of foldp doesn't mention that strangeness
3. the current documentation of foldp doesn't justify that strangeness

All I can do is address 2. here. If 1. and/or 3. should be addressed, that should be brought up on the mailing list or in separate issues in this repository.

If there is some way to address 2. better than I have in the commit above, I'm all ears.

[jvoigtlaender]

To take some pull request pressure off this repository, I am now going to merge some PRs that are "just" about documentation and that seem uncontroversial. I am starting with this one, which I consider uncontroversial (in as far as its stated goal, namely addressing point 2. from the previous comment), because it was already once merged in by Evan (see link above).

Any other points, specifically 1. and 3. from the previous comment, could be the topic of separate issues/PRs.

[jvoigtlaender]

To take some pull request pressure off this repository, I am now going to merge some PRs that are "just" about documentation and that seem uncontroversial. I am starting with this one, which I consider uncontroversial (in as far as its stated goal, namely addressing point 2. from the previous comment), because it was already once merged in by Evan (see link above).

Any other points, specifically 1. and 3. from the previous comment, could be the topic of separate issues/PRs.