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

Fix lsi.rb Comparable#== warning #33

Merged
merged 1 commit into from
Jun 13, 2015
Merged

Fix lsi.rb Comparable#== warning #33

merged 1 commit into from
Jun 13, 2015

Conversation

slashfoo
Copy link
Contributor

Warnings produced when doing jekyll build were:

warning: Comparable#== will no more rescue exceptions of #<=> in the next release.
warning: Return nil in #<=> if the comparison is inappropriate or avoid such comparison.

The warnings are supressed when using .eql? method.

jekyll (2.5.3)
classifier-reborn (2.0.3)
rb-gsl (1.16.0.4)

Warnings produced when doing `jekyll build` were:

warning: Comparable#== will no more rescue exceptions of #<=> in the next release.
warning: Return nil in #<=> if the comparison is inappropriate or avoid such comparison.

The warnings are supressed when using .eql? method.

jekyll (2.5.3)
classifier-reborn (2.0.3)
rb-gsl (1.16.0.4)
@parkr
Copy link
Member

parkr commented Feb 21, 2015

Not entirely sure it still hits #<=> with #eql?. Worried this breaks things. Will have to look into it further.

@slashfoo
Copy link
Contributor Author

Cool, were you able to reproduce the issue on your end? I'm not entirely familiar, I just noticed the warning and tried to fix it and report.

@Ch4s3
Copy link
Member

Ch4s3 commented Feb 21, 2015

I'm playing around with this as well. That deprecation warning is popping up everywhere lately :(

.eql? is implemented as follows:

               static VALUE
rb_str_eql(VALUE str1, VALUE str2)
{
    if (str1 == str2) return Qtrue;
    if (!RB_TYPE_P(str2, T_STRING)) return Qfalse;
    return str_eql(str1, str2);
}

whereas == and === are implemented as:

               VALUE
rb_str_equal(VALUE str1, VALUE str2)
{
    if (str1 == str2) return Qtrue;
    if (!RB_TYPE_P(str2, T_STRING)) {
        if (!rb_respond_to(str2, rb_intern("to_str"))) {
            return Qfalse;
        }
        return rb_equal(str2, str1);
    }
    return str_eql(str1, str2);
}

The deprecation warning is about swallowing errors with respect to different or lacking implementation of <=>. I'm concerned that .eql? may throw the same warning under some circumstances. I think it warrants a thorough investigation.

@slashfoo
Copy link
Contributor Author

ok, do you want me to cancel the pull request and open an issue instead then and track everything there, or leave this open until we can figure this one out?

Note: I included myself in the we, as I'll try to help as much as I can.

@parkr
Copy link
Member

parkr commented Feb 23, 2015

Let's just leave this one open :)

@parkr parkr self-assigned this Feb 23, 2015
@parkr
Copy link
Member

parkr commented Mar 1, 2015

Ok, I'm cool on this one, I think. Let's add a test or two to make sure it works with a custom #<=>.

@Ch4s3 Ch4s3 mentioned this pull request Apr 13, 2015
@Ch4s3
Copy link
Member

Ch4s3 commented Jun 12, 2015

Going to try to test this and merge it today.

@Ch4s3
Copy link
Member

Ch4s3 commented Jun 13, 2015

@parkr I was looking at this yesterday, what do you want to do in terms of testing for custom #<=>? I'm happy to take care of that but not sure what to do here.

@parkr
Copy link
Member

parkr commented Jun 13, 2015

Do we have any tests for find_related? I suppose as long as the tests return the same thing, then we're ok. Is pair[0] always a string?

@Ch4s3
Copy link
Member

Ch4s3 commented Jun 13, 2015

It's pretty well covered. And, as far as I can tell any case that could lead to pair[0] not being a string would explode higher up the stack. So, I think this pr is good to go.

Note, this caused me to pour over the tests again, and one of my goals in the future is to pour some work into improving the test suite, but that isn't especially germane.

@parkr
Copy link
Member

parkr commented Jun 13, 2015

Cool, thanks @Ch4s3! :shipit:

Ch4s3 added a commit that referenced this pull request Jun 13, 2015
Fix lsi.rb Comparable#== warning
@Ch4s3 Ch4s3 merged commit faa1b10 into jekyll:master Jun 13, 2015
Ch4s3 added a commit that referenced this pull request Jun 13, 2015
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.

None yet

3 participants