-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
WIP better error msg #6751
base: main
Are you sure you want to change the base?
WIP better error msg #6751
Conversation
@@ -1675,7 +1675,11 @@ mod test_reporting { | |||
|
|||
Roc does not allow functions to be partially applied. Use a closure to | |||
make partial application explicit. | |||
" | |||
|
|||
For example: ["a", "b"] |> \list -> Str.joinWith list ", " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea! What if we give an example that doesn't have a simplified version, so there's no need for the second line? For example:
List.map nums \num -> num + 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rtfeldman Thanks for replying!
In regard to your proposal:
In your example, the closure (lambda) means a different thing comparing to the source of error.
Here is the original error message taken from the tests:
── TOO FEW ARGS ────────────────────────────────────────────────────────────────
The add function expects 2 arguments, but it got only 1:
4│ Num.add 2
^^^^^^^
Roc does not allow functions to be partially applied. Use a closure to
make partial application explicit.
My first attempt at providing the example would be:
For example: \n -> Num.add n 2
It is a good illustration, but if I substitute it into Num.add 2
it does not make sense.
That's why it may be helpful to provide the valid context for the closure, e.g. the pipe operation.
The new idea:
I think we don't need a context if we demonstrate the problem and solution side-by-side.
For example instead of `Num.add 2` use `\n -> Num.add n 2`
@rtfeldman What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting!
Thinking about it some more, it occurs to me that people who encounter this message might be in one of a few different scenarios:
- They forgot an argument and are going to add it
- They're used to languages where arguments can be omitted (e.g. in JavaScript you can leave off an argument and the argument will be
undefined
in the function body) - They're used to curried languages and expected this to result in partial application
I think a lot of people reading the error message in the first two scenarios won't know what "partial application" means, and would actually have a better experience if we didn't bring that up at all - because they might wonder "wait, is partial application something I should be doing here?" when actually they just forgot an argument.
Here's an idea for an error message that might be clearer for people in different scenarios.
Today:
The `f` function expects 2 arguments, but it got only 1:
7│ f 1
^
Roc does not allow functions to be partially applied. Use a closure
to make partial application explicit.
Idea:
The `f` function expects 2 arguments, but it got only 1:
7│ f 1
^
Calling a Roc function requires providing all of its arguments.
I think this wording would be clear for people in all 3 scenarios:
- Forgot an argument: it's clear that they forgot an argument and need to provide it, and it doesn't bring up new terminology that might lead them down the wrong path
- Thought arguments might be optional: makes it clear that all arguments are required
- Expected partial application: makes it clear that all arguments are required
I think the number of people who know what partial application is but don't know how to do it by hand will be very low, so I think this might end up being the most helpful message to people encountering it from different backgrounds!
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I agree that the words of partial application
should be set aside and more "based" explanation provided: all arguments should be provided
looks good to me.
But we still have 2 different contexts for this error: standalone function call and piping.
The problem that the solution hint for the error is different depending on context:
- Provide all arguments for function call
- Use the closure syntax to adapt the function call to the required number of arguments
Idea 1 - Split the error in two
Pros:
- It is fewer words to read
- Focused on the right solution for the usage context
Cons:
- Complicates the compiler implementation
- Reverse of the focused - it probably useful to see the (only) 2 ways of solving such problems in general
Idea 2 - Keep a single error but provide 2 possible context+solutions
Pros:
- Complete info package
- Just change the error message
Cons:
- Too long to read (is it really a problem ???)
- Requires from the user to select appropriate context (hey, there are only 2)
Ok. I am biased :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we still have 2 different contexts for this error: standalone function call and piping.
[...]
Use the closure syntax to adapt the function call to the required number of arguments
Ah, so the current advice of "Use a closure to make partial application explicit" is not about pipelines; it's intended to be a message for people who expect functions to be curried.
In other words, in curried languages I can do List.map nums (Num.add 1)
and Num.add
would be partially applied with 1
, but in Roc I would need to "Use the closure syntax to adapt the function call to the required number of arguments" - in other words, write it as List.map nums \num -> Num.add 1 num
instead of List.map nums (Num.add 1)
(which would produce an error like this today).
Does that make sense?
Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked |
This reverts commit ea23576.
TL;DR;
The PR adds the usage examples into the error message for
TOO FEW ARGS
and adjusts the tests.Explanation
The PR is improving the error message for the too few arguments to a function by adding the examples into the message, specifically in the piping context.
I am learning the Roc language and trying to find analogs from my background (C# and a bit of F#, Java, Scala).
Trying the pipe operator to replicate C# LINQ (collection processing), I was confused by the error message:
I know what the closure is generally, but what does it mean to
make partial application explicit
.There are too many arbitrary words glued together with the meaning lost on me, sorry :)
And what should I do practically to move on?
So the examples may help me to unblock and illustrate explicit closures/lambdas and partial application.
Like this: