-
Notifications
You must be signed in to change notification settings - Fork 455
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
Try making top level unit error message clearer #6407
Conversation
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.
sweet!
jscomp/build_tests/super_errors/expected/partial_app.res.expected
Outdated
Show resolved
Hide resolved
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.
this is quite clear, I like the fact you customize the error message depending on the type, good job!
🙏 Are there more scenarios where we should customize this error message depending on what the actual expression is? I couldn't think of any that doesn't also fit well as just a generic message. Function calls seems to be the main gotcha here, because that's probably what you're doing most of the time as you're getting this message. |
Clear msg! 👍 |
| Bs_toplevel_expression_unit -> | ||
"Toplevel expression is expected to have unit type." | ||
| Bs_toplevel_expression_unit help -> | ||
Printf.sprintf "This%sis at the top level and is expected to return `unit`. But, it's returning %s.\n\n In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function.%s" |
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.
Sorry for reviewing so late 😞 and for nit-picking, but I think there should be no comma after "But".
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.
PRs welcome for late reviewers 😏
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.
Done in #6409
No description provided.