Return nil when comparing a Post with a non-Post.
#3484
Conversation
In Ruby < 2.2, `==` will swallow any exceptions raised by `<=>` and return nil. In Ruby > 2.2, `==` will surface any exceptions raised by `<=>`. In Ruby 2.2, `==` will show an annoying warning message if `<=>` raises an exception, which drowns out most of Jekyll's output. This patch double-checks that the `date` and `slug` methods exist on whatever you're comparing a `Post` to, and returns `nil` if they don't. This keeps compatibility with Ruby 2.2+.
|
I'm cool with this. How does this affect sorting if a @mattr- ? |
| @@ -155,6 +155,9 @@ def relative_path | |||
| # | |||
| # Returns -1, 0, 1 | |||
| def <=>(other) | |||
| return unless other.respond_to?(:date) && | |||
There was a problem hiding this comment.
Can we rescue the exception instead?
def <=>(other)
# existing code
rescue
nil
end
There was a problem hiding this comment.
Looking into it further, it seems like there's a bug that was being masked in earlier versions of Ruby. Could you take a look at the pull request conversation? I have a fix, I'm just not sure how to put together a test for it.
|
If we want to make sure we don't affect sorting, then we'd want to compact any arrays that have Granted, the example above is really contrived, but ¯_(ツ)_/¯ |
|
So, after sleeping on it, I think this is the wrong solution -- it's hiding something that might actually be a bug. So I changed the line that threw the warning (from the LSI gem): proximity_array_for_content( doc, &block ).reject { |pair| pair[0] == doc }to proximity_array_for_content( doc, &block ).reject { |pair| (pair[0] <=> doc) == 0 }to show the actual error. And it turns out it's not comparing to nil. It looks like it's trying to compare a I can try to take another stab at this and figure out why. But I'd appreciate any insight if you have it! |
|
Ah, this might be why. Jekyll is indexing posts: site.posts.each do |x|
lsi.add_item(x)
endBut finding related posts based on content: def lsi_related_posts
self.class.lsi.find_related(post.content, 11) - [post]
end |
|
well, that definitely seems like a source of issues. 😃 What happens if you change it to find related posts based on the post object itself? |
|
Changing def lsi_related_posts
self.class.lsi.find_related(post, 11)
enddoesn't seem to cause any problems. It's still excluding the current post from the related posts, and the related posts seem as reasonable as a magical black-box classifier can give you. What kind of test would you want to see for a change like this? I could mock it, but that seems like it might be too implementation-focused. |
|
Two posts later, things still seem to be working well with that |
When looking for related posts, Jekyll was indexing `Jekyll::Post` objects, but finding related posts based on `Jekyll::Post#content`. This caused two problems: 1. Ruby 2.2 will warn on == if <=> throws an exception (and future Ruby versions will surface that exception). Because `String`s can't be compared with `Jekyll::Post`s, this warning was appearing all the time while searching for related posts. 2. LSI won't return a post itself when searching for related posts. But LSI could never tell that we were searching on a post, since Jekyll passed post content, not a post object. With this fix, we can remove the `- [post]` from `Jekyll::RelatedPosts#find_related`. This is a more accurate fix for jekyll#3484.
|
This PR is superseded by #3629. |
In Ruby < 2.2,
==will swallow any exceptions raised by<=>andreturn nil.
In Ruby > 2.2,
==will surface any exceptions raised by<=>.In Ruby 2.2,
==will show an annoying warning message if<=>raisesan exception, which drowns out most of Jekyll's output.
This patch double-checks that the
dateandslugmethods exist onwhatever you're comparing a
Postto, and returnsnilif theydon't. This keeps compatibility with Ruby 2.2+.
The Ruby change was discussed here:
https://bugs.ruby-lang.org/issues/7688, and this patch was inspired by
the one @tenderlove made to rdoc here:
https://github.com/rdoc/rdoc/commit/23c276de
Why LSI has nil documents it's comparing posts to is beyond me, but this
patch will do the right thing when it does!