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

Enable incremental operation #58

Merged
merged 8 commits into from
Jan 17, 2017
Merged

Enable incremental operation #58

merged 8 commits into from
Jan 17, 2017

Conversation

mnot
Copy link
Contributor

@mnot mnot commented Feb 15, 2016

I put this together quickly, seems to work for me. May need to track other dependencies.

@alfredxing
Copy link
Member

Thanks for the pull request! If I remember correctly, the reason I disabled incremental regen completely was because it still doesn't handle the case where posts are added—that is, while the archives page will be updated with your code, it will not be updated when a new post is created.

@mnot
Copy link
Contributor Author

mnot commented Feb 23, 2016

I tested that case with this patch, works for me.

@mnot
Copy link
Contributor Author

mnot commented Feb 24, 2016

Updated to only write the archives when they change.

Bit of a messy commit history, sorry.

@parkr
Copy link
Member

parkr commented May 17, 2016

@mnot Would you mind looking into the failures?

@toolness
Copy link

Hey @parkr, not sure if github notified you of the new commits that @mnot just added, but I figured I'd ping you here just in case it didn't--looks like this PR is passing all travis checks!

@parkr
Copy link
Member

parkr commented May 26, 2016

@toolness this is really @alfredxing's domain 😄

@parkr
Copy link
Member

parkr commented May 26, 2016

@alfredxing I added the @jekyll/archives team which grants members write access to this repo. Feel free to invite anyone you wish!

@toolness
Copy link

toolness commented May 26, 2016

Oh lol, sorry, that was a brain fart on my part :)

Thanks for working on this btw @mnot! Really really helpful PR. 😁

@mnot
Copy link
Contributor Author

mnot commented May 27, 2016

Please do have a good look through before you merge; while it works for me, I've also never really touched Ruby before, so I'm unfamiliar with the idioms, etc.

@gboone
Copy link

gboone commented Aug 23, 2016

We'd really love to implement this change in our project at 18F/beta.18f.gov. What's the status on this request?

@parkr
Copy link
Member

parkr commented Aug 23, 2016

On the surface, this LGTM.

@alfredxing
Copy link
Member

Looks mostly good! Just one question: why keep track of dependencies for archive pages? Most likely we'll need regenerate: true on them in case posts are added?

@mnot
Copy link
Contributor Author

mnot commented Sep 2, 2016

That avoids regenerating all of your archives (e.g., all categories, all monthly archives back for however long, etc.) for each build.

@gemfarmer
Copy link

gemfarmer commented Oct 13, 2016

@mnot Is this still being worked on?

@alfredxing
Copy link
Member

I just took some time and tried out a bunch of scenarios with this patch, and everything seems good!

However, I'm not 100% confident that this is going to work all of the time (without major bugs)... I'm thinking about maybe adding another config option (eww) to disable incremental regen? Not sure how you all feel about this (@parkr?)

@parkr
Copy link
Member

parkr commented Oct 18, 2016

@alfredxing At this point, I'd say try it. Worst case scenario, they run a full rebuild and the problem ceases. Time to rip off the bandaid 😃

@alfredxing
Copy link
Member

Okie!

@mnot There are a couple of conflicts with my recent Page migration. I looked at them locally and they seemed easy to resolve. Can you rebase and take a look? 🙇

@mnot
Copy link
Contributor Author

mnot commented Oct 18, 2016

Sure. Might take a week or so, have a few things I need to do first.

# Conflicts:
#	lib/jekyll-archives.rb
#	lib/jekyll-archives/archive.rb
@mnot
Copy link
Contributor Author

mnot commented Nov 3, 2016

Hmm, that problem with 1.9 seems to be an environment issue...

@parkr
Copy link
Member

parkr commented Nov 3, 2016

@mnot You're right! I'd say we can drop support for Ruby 1.9 personally, but that's not really up to me here. @alfredxing can decide that. From the build error, it looks like we're trying to use Octokit 4.4.1, but Octokit 4.0 and above require Ruby 2.0 and above.

@paulrobertlloyd
Copy link
Contributor

paulrobertlloyd commented Jan 15, 2017

Reading through this conversation, seems like there’s little preventing this PR being accepted. Would be nice to make this plugin compatible with incremental rebuilds. Any possibility of making the final push!?

@parkr
Copy link
Member

parkr commented Jan 17, 2017

Let's get this show on the road. @alfredxing, I'm going to merge this for now. Please file any issues/PR's if you'd like to see anything here change.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit b7d4e1a into jekyll:master Jan 17, 2017
jekyllbot added a commit that referenced this pull request Jan 17, 2017
@paulrobertlloyd
Copy link
Contributor

Yay! Thanks @parkr (and @mnot of course)!

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.

8 participants