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 comparison error message. #392

Open
wants to merge 1 commit into
base: trunk
from

Conversation

3 participants
@ernie
Copy link

ernie commented Sep 7, 2013

In certain cases, things like Array#sort can result in a confusing error
message. For instance where a and b are characters in a string,
"string":

array.sort { |a, b| string.index(a) <=> string.index(b) }

If one of the index calls returns nil, we will get "comparison of String
with String failed", which is somewhat unhelpful, since it's easy to be
confused, given that what is really being compared is a Fixnum or
NilClass (the cause of the error). Yes, as far as Array#sort is
concerned, the two characters are the things being sorted, but it's
useful to call attention to the return value of the comparison in this
case.

This patch adds a "reason" argument to rb_cmperr, which will provide an
error message of "comparison of String with String failed: comparator
returned nil" in the case above, or, in the case of:

1.upto('10').to_a

it will provide the message: "comparison of Fixnum with String failed:
coercion was not possible"

Improve comparison error message.
In certain cases, things like Array#sort can result in a confusing error
message. For instance where a and b are characters in a string,
"string":

  array.sort { |a, b| string.index(a) <=> string.index(b) }

If one of the index calls returns nil, we will get "comparison of String
with String failed", which is somewhat unhelpful, since it's easy to be
confused, given that what is really being compared is a Fixnum or
NilClass (the cause of the error). Yes, as far as Array#sort is
concerned, the two characters are the things being sorted, but it's
useful to call attention to the return value of the comparison in this
case.

This patch adds a "reason" argument to rb_cmperr, which will provide an
error message of "comparison of String with String failed: comparator
returned nil" in the case above, or, in the case of:

  1.upto('10').to_a

it will provide the message: "comparison of Fixnum with String failed:
coercion was not possible"
@zzak

This comment has been minimized.

Copy link
Member

zzak commented Oct 15, 2013

@nobu any objection here?

@marcandre

This comment has been minimized.

Copy link
Member

marcandre commented Oct 15, 2013

Idea is good. Not too sure about the wording of "coercion was not possible". Do we need more explanation in that case?

@ernie

This comment has been minimized.

Copy link
Author

ernie commented Oct 15, 2013

Not sure how much more specific we can get without making the change somewhat more complex. Open to making whatever changes core seems appropriate in order to get this merged, though!

@ernie

This comment has been minimized.

Copy link
Author

ernie commented Jan 17, 2014

Just thought I'd check in on this. Any word?

@sorah sorah force-pushed the ruby:trunk branch from 5317131 to 307198a Nov 26, 2014

@matzbot matzbot force-pushed the ruby:trunk branch from 2677ddd to ce7ad3a Jan 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment