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

Improve documentation of `Debug.watch` #264

Merged
merged 2 commits into from Jun 29, 2015

Conversation

Projects
None yet
3 participants
@jvoigtlaender
Contributor

jvoigtlaender commented Jun 6, 2015

Specifically, it is tempting to call Debug.watch directly on a signal, which is not useful though. That can lead to puzzlement, since it then seems as if Debug.watch serves no purpose. See this question on the mailing list. The expanded documentation should make clear now that one needs to map Debug.watch into the signal if one wants to observe the value carried on the signal.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jun 6, 2015

Contributor

I wonder whether it would make sense to even make it illegal to call Debug.watch on something of Signal-type. Could be made a runtime error. Should be unproblematic since Debug.watch shouldn't be used in production anyway. And calling it on a signal is never what you want (because it has no effect). So the debugger would save you some trouble if it simply errored out (with a helpful message) if you accidentally call Debug.watch on a Signal-typed thing.

Contributor

jvoigtlaender commented Jun 6, 2015

I wonder whether it would make sense to even make it illegal to call Debug.watch on something of Signal-type. Could be made a runtime error. Should be unproblematic since Debug.watch shouldn't be used in production anyway. And calling it on a signal is never what you want (because it has no effect). So the debugger would save you some trouble if it simply errored out (with a helpful message) if you accidentally call Debug.watch on a Signal-typed thing.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jun 6, 2015

Contributor

Same (concerning the proposed runtime error) for Debug.watchSummary, of course.

Contributor

jvoigtlaender commented Jun 6, 2015

Same (concerning the proposed runtime error) for Debug.watchSummary, of course.

Show outdated Hide outdated src/Debug.elm
`Debug.watch` allows us to name the value and show it with the debugger.
`Debug.watch` allows us to name the value and show it with the debugger. The
result of evaluating such an expression is exactly the same as if not having
the call to `Debug.watch` at all.

This comment has been minimized.

@mgold

mgold Jun 8, 2015

Contributor

Remove "if". Also, "the result of evaluating such an expression" is a bit technical. Maybe just "the value is returned unchanged"?

@mgold

mgold Jun 8, 2015

Contributor

Remove "if". Also, "the result of evaluating such an expression" is a bit technical. Maybe just "the value is returned unchanged"?

Show outdated Hide outdated src/Debug.elm
Note that calling `Debug.watch` on a signal is not useful. Instead, it needs
to be mapped into the signal (to act on the contained value). So if you want
to watch a timer signal, instead of `Debug.watch "time" <| Time.every second`

This comment has been minimized.

@mgold

mgold Jun 8, 2015

Contributor

Both every and second are in the Time module so it doesn't make much sense to have one qualified and one not. For the sake of explanation I would drop the Time. part.

@mgold

mgold Jun 8, 2015

Contributor

Both every and second are in the Time module so it doesn't make much sense to have one qualified and one not. For the sake of explanation I would drop the Time. part.

That means it's easy to add `Debug.watch` to any value.
Note that calling `Debug.watch` on a signal is not useful. Instead, it needs
to be mapped into the signal (to act on the contained value). So if you want

This comment has been minimized.

@mgold

mgold Jun 8, 2015

Contributor

mapped into

onto? Maybe that's just me.

@mgold

mgold Jun 8, 2015

Contributor

mapped into

onto? Maybe that's just me.

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold Jun 8, 2015

Contributor

Comments inline; hopefully I'm not too nitpicky. I like where this is going.

Contributor

mgold commented Jun 8, 2015

Comments inline; hopefully I'm not too nitpicky. I like where this is going.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jun 8, 2015

Contributor

I reformulated a few bits. I do think that "mapped into" is better here.

Contributor

jvoigtlaender commented Jun 8, 2015

I reformulated a few bits. I do think that "mapped into" is better here.

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold Jun 9, 2015

Contributor

I won't fight you on it then. Should be good for whenever Evan gets back from his world tour.

Contributor

mgold commented Jun 9, 2015

I won't fight you on it then. Should be good for whenever Evan gets back from his world tour.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jun 29, 2015

Member

Looks great to me, thank you @jvoigtlaender!

Member

evancz commented Jun 29, 2015

Looks great to me, thank you @jvoigtlaender!

evancz pushed a commit that referenced this pull request Jun 29, 2015

Merge pull request #264 from jvoigtlaender/patch-1
Improve documentation of `Debug.watch`

@evancz evancz merged commit e59e84e into elm:master Jun 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jvoigtlaender jvoigtlaender deleted the jvoigtlaender:patch-1 branch Jun 30, 2015

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