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

Improve compile time error for #sort(&block : T, T -> U) #14693

Merged

Conversation

beta-ziliani
Copy link
Member

Little improvement to reference Comparable#<=> in sort operations. Ref: https://forum.crystal-lang.org/t/sort-by-time/6918/1

@crysbot
Copy link

crysbot commented Jun 11, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/sort-by-time/6918/3

Copy link
Contributor

@robacarp robacarp left a comment

Choose a reason for hiding this comment

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

Thank you for helping provide direction to future wandering programmers -- admittedly, I'll probably end up bumping up against this exact message again in the future! 😄

src/array.cr Outdated
@@ -1680,7 +1680,7 @@ class Array(T)
# Raises `ArgumentError` if for any two elements the block returns `nil`.
def unstable_sort(&block : T, T -> U) : Array(T) forall U
{% unless U <= Int32? %}
{% raise "Expected block to return Int32 or Nil, not #{U}" %}
{% raise "Expected block to return Int32 or Nil, not #{U} (see Comparable#<=>)" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if stdlib has a policy on tone for error messages or not, so please forgive the suggestion if I'm out of step. I had been planning to submit a pull like this, but admittedly wouldn't have been as comprehensive as you have been here.

Looking at the docs for Comparable#<=>, I'm not sure it would have actually helped me get what I wanted out of the situation. What do you think about an error message which is a little more prescriptive? Something like "did you mean to use #sort_by?" (or unstable_sort_by, sort_by!, etc).

Copy link
Member

@straight-shoota straight-shoota Jun 11, 2024

Choose a reason for hiding this comment

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

I'd agree we could be a bit more elaborate.
WDYT about this?

Suggested change
{% raise "Expected block to return Int32 or Nil, not #{U} (see Comparable#<=>)" %}
{% raise "Expected block to return Int32 or Nil, not #{U}.\nThe block is supposed to be a custom comparison operation, compatible with `Comparable#<=>`.\nDid you mean to use `#sort_by?`" %}

Copy link
Contributor

@Sija Sija Jun 11, 2024

Choose a reason for hiding this comment

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

Do multiline compiler errors render well?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that should be fine.

$ crystal eval '{% raise "Expected block to return Int32 or Nil.\nThe block is supposed to be a custom comparison operation, compatible with `Comparable#<=>`.\nDid you mean to use `#sort_by?`" %}'

Showing last frame. Use --error-trace for full trace.

error in line 1
Error: Expected block to return Int32 or Nil, not.

The block is supposed to be a custom comparison operation, compatible with `Comparable#<=>`.
Did you mean to use `#sort_by?`

@straight-shoota straight-shoota added this to the 1.13.0 milestone Jun 13, 2024
@straight-shoota straight-shoota changed the title Better error reporting for sort Improve compile time error for #sort(&block : T, T -> U) with mismatching U Jun 14, 2024
@straight-shoota straight-shoota changed the title Improve compile time error for #sort(&block : T, T -> U) with mismatching U Improve compile time error for #sort(&block : T, T -> U) Jun 14, 2024
@straight-shoota straight-shoota merged commit c2dd548 into crystal-lang:master Jun 14, 2024
60 of 61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants