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

Move the building of related posts into their own class #1057

Merged
merged 7 commits into from May 15, 2013

Conversation

Projects
None yet
4 participants
@mattr-
Member

mattr- commented May 8, 2013

This is a WIP extraction of the related posts functionality into its own class, with the eventual goal of removing the Post#related_posts method entirely.

Why remove Post#related_posts? The reason is because it's only use is to provide a related_posts method for the site object in a post's liquid object.

@@ -1,3 +1,5 @@
require 'jekyll/related_posts'

This comment has been minimized.

@parkr

parkr May 8, 2013

Member

This should go into lib/jekyll.rb :)

@parkr

This comment has been minimized.

Member

parkr commented May 8, 2013

This is awesome!!

mattr- added some commits May 8, 2013

Wrap tests around Jekyll::RelatedPosts
This gives me more confidence that we're doing the right things when it
comes to both the LSI and non-LSI cases and prevents regressions.
Update the display of the LSI progress output
It now fits more in line with what the other messages display.
@mattr-

This comment has been minimized.

Member

mattr- commented May 9, 2013

Here's what the new output from 08b49ec looks like
screen shot 2013-05-08 at 9 59 00 pm

@mattr-

This comment has been minimized.

Member

mattr- commented May 9, 2013

I think I may have to stub out the classifier more because it's really slow unless you have rb-gsl installed.

Also stub the building of the index
Since we don't actually use the index in getting the related posts from
the tests there's no need to build an index, which can take a long time
if the ruby bindings for the GSL library are not installed.
@parkr

This comment has been minimized.

Member

parkr commented May 9, 2013

This is looking awesome, nice work!

@parkr

This comment has been minimized.

Member

parkr commented May 13, 2013

@mattr- What's the status of this? Ready to merge it?

display("\n Populating LSI... ")
self.site.posts.each do |x|
lsi.add_item(x)

This comment has been minimized.

@parkr

parkr May 13, 2013

Member

No more status printing here? If it takes a while, it might be good to give the user some hint as to how for along it is.

This comment has been minimized.

@mattr-

mattr- May 15, 2013

Member

The adding of the items that the index should be generated from is not the slow part here. The building of the index is and, sadly, we don't have any way to hook into the progress of that. The extra dots that went by super fast when adding items just seemed to add clutter, so I left them out.

def build_index
self.class.lsi ||= begin
lsi = Classifier::LSI.new(:auto_rebuild => false)
display("\n Populating LSI... ")

This comment has been minimized.

@parkr

parkr May 13, 2013

Member

Jekyll::Logger has a message - .formatted_topic that rjust's it tot the proper place.

display("\n#{Jekyll::Logger.formatted_topic('Populating LSI...')}")
@mattr-

This comment has been minimized.

Member

mattr- commented May 13, 2013

I'll make these changes and then take another look. There's some additional smells in here that I may want to address but I may just make those changes later.

@parkr

This comment has been minimized.

Member

parkr commented May 13, 2013

Ok! We can always use Code Climate to help sniff out those smells, too! https://codeclimate.com/github/mojombo/jekyll

@mattr-

This comment has been minimized.

Member

mattr- commented May 13, 2013

Not sure if CodeClimate will find these smells that I'm talking about but
we shall see once this is merged. 😃

On Mon, May 13, 2013 at 12:00 PM, Parker Moore notifications@github.comwrote:

Ok! We can always use Code Climate to help sniff out those smells, too!
https://codeclimate.com/github/mojombo/jekyll


Reply to this email directly or view it on GitHubhttps://github.com//pull/1057#issuecomment-17825145
.

@parkr

This comment has been minimized.

Member

parkr commented May 13, 2013

You're probably right :) What the human mind can achieve, few consumer-level services can provide!

@mattr-

This comment has been minimized.

Member

mattr- commented May 15, 2013

I think this is ready to merge now. I'll make the other changes at a later date.

@parkr

This comment has been minimized.

Member

parkr commented May 15, 2013

Awesome, LGTM. 1.0.3 or 1.1, you think?

@mattr-

This comment has been minimized.

Member

mattr- commented May 15, 2013

I think 1.0.3 should be fine. I don't think there's anything in here that
makes it a 1.1 change.

parkr added a commit that referenced this pull request May 15, 2013

Merge pull request #1057 from mojombo/refactor-related-posts
Move the building of related posts into their own class

@parkr parkr merged commit ef48cbe into master May 15, 2013

1 check passed

default The Travis CI build passed
Details

@parkr parkr deleted the refactor-related-posts branch May 15, 2013

parkr added a commit that referenced this pull request May 15, 2013

@parkr

This comment has been minimized.

Member

parkr commented May 15, 2013

💯 🎆

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented May 16, 2013

Not a big issue, but I think it's this PR that makes the following appear while running the unit tests:

Populating LSI...
Rebuilding index...

@mattr-

This comment has been minimized.

Member

mattr- commented May 16, 2013

It is. We were never testing related post generating with LSI indexing
before now.

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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