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

Try making top level unit error message clearer #6407

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

zth
Copy link
Collaborator

@zth zth commented Sep 18, 2023

No description provided.

@zth zth requested a review from cristianoc September 18, 2023 19:51
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet!

Copy link
Contributor

@tsnobip tsnobip left a 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!

@zth
Copy link
Collaborator Author

zth commented Sep 19, 2023

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.

@mununki
Copy link
Member

mununki commented Sep 19, 2023

Clear msg! 👍

@zth zth merged commit 9d7f2c2 into master Sep 19, 2023
@zth zth deleted the improve-toplevel-unit-error-msg branch September 19, 2023 07:43
| 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"
Copy link
Member

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".

Copy link
Collaborator Author

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 😏

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #6409

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.

6 participants