Skip to content
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

ch11: fix saveComment definition #500

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eush77
Copy link
Contributor

@eush77 eush77 commented Feb 15, 2019

Hi,

Function saveComment in this chapter is defined as a composition of functions, one of which is getValue('#comment'). However, getValue('#comment') is not a function according to its type specification:

// getValue :: Selector -> Task Error (Maybe String) 

This patch fixes the definitions of saveComment to avoid type mismatch.

Function `saveComment` in this chapter is defined as composition of
functions, one of which is `getValue('#comment')`. However,
`getValue('#comment')` is not a function according to its type
specification:

```js
// getValue :: Selector -> Task Error (Maybe String)
```

This fixes the definitions of `saveComment` to avoid type mismatch.
@KtorZ
Copy link
Member

KtorZ commented Feb 18, 2019

@eush77
Copy link
Contributor Author

eush77 commented Feb 18, 2019 via email

@KtorZ
Copy link
Member

KtorZ commented Feb 18, 2019

I am not sure to follow you on that one. We have:

always :: a -> b -> a
getValue :: Selector -> Task Error (Maybe String)
getValue("#comment") :: Task Error (Maybe String)

Hence:

always(getValue("#comment")) :: b -> Task Error (Maybe String)

Which should run perfectly fine in the pipeline; it's an unary function returning a Task.

@eush77
Copy link
Contributor Author

eush77 commented Feb 18, 2019

it's an unary function returning a Task

Exactly, but as I recall from reading the book () in the type signature means that the function is nullary, meaning you call it like saveComment(), but in this case it would still return a function due to implementation details of curry:

always(getValue("#comment"))() :: b -> Task Error (Maybe String)

I may be wrong about the () thing though, but I'm sure I've seen it used like that, either in the book examples or in the exercises.

@KtorZ
Copy link
Member

KtorZ commented Feb 18, 2019

Hmmm. That is incorrect. The type for always(getValue("#comment"))() is actually Task Error (Maybe String). Having Unit (a.k.a ()) in a type signature just means that it only accepts one value which is precisely Unit. That's a type of cardinality 1 (there's only one possible value, which is just plain void).

Functions don't usually take () as an argument, because it means that the argument is superfluous (there's nothing you can do with.. nothing). Still, a function written as () -> a still has one argument, which has to be Unit / void. In our case above, it's slightly more general than that because we don't have to require the argument to be unit; we ignore it anyway, so it could just be anything (hence the b).

@eush77
Copy link
Contributor Author

eush77 commented Feb 18, 2019

The type for always(getValue("#comment"))() is actually Task Error (Maybe String)

It's not, and you can try it to see for yourself:

> always(Task.of(1))
[Function: always]
> always(Task.of(1))()
[Function: always]
> always(Task.of(1))(undefined)
Task(?)

Still, a function written as () -> a still has one argument, which has to be Unit / void.

Translating to JavaScript, this means that () is a type consisting of a single value undefined, and all call sites must pass it explicitly. Perhaps there should be a paragraph somewhere in chapter 7 introducing this type before it's first used in chapter 8.

So the functions in this example must be called like always(getValue("#comment"))(undefined) and saveComment(undefined), and thus always can indeed be used here to make the example point-free. I have just made this change in my branch.

I also found a couple of places where a function is called with no arguments, but I guess I will make another PR to fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants