Remove deprecated defs

[paulrobertlloyd]

Reviewing the commit history of this plugin, it seems that somewhere between #67 and #58 being merged in, deprecated code that was removed was added back in again later.

The first merge, in which Archive became a subclass of Page, removed defs that were no longer required as part of this change. The merge that appears to reintroduce them is the one adding support for incremental rebuilds.

Have I read this correctly? Can we delete some code?!

[paulrobertlloyd]

What’s the best means of requesting review for changes on this project? Feel bad always pinging @parkr and @alfredxing (and it seems that non-team members aren’t able to ping the @jekyll/archives team address 😞)

[pathawks]

/cc: @jekyll/archives

[pathawks]

It seems that @alfredxing is currently the only member of @jekyll/archives

[alfredxing]

Hmm, this is probably because #58 was merged after #67...

@paulrobertlloyd Have you tested this with incremental regeneration? I'm worried about the removal of this line, since as far as I know that's the only place add_dependencies is called.

[paulrobertlloyd]

@alfredxing Testing… good point! I’ve tested this change with my own site build and, going by the fact that I can tell if incremental regeneration is working as my rebuild time goes from ~30s to ~8s (!), removing these lines had no negative impact. In fact, your comment made me question whether I could also remove the add_dependencies and regenerate? defs too (these were also added in #58, and appear related to incremental regeneration). This change also had no negative impact.

Possible conclusion: making Archive a subclass of Page was the ‘correct’ means of making this plugin compatible with incremental rebuilds (as regeneration behaviour could then be inherited from Jekyll core). This isn’t to denigrate @mnot’s work, although I feel bad deleting many of Mark’s additions 😳. I don’t feel I have enough knowledge of Ruby or Jekyll to make this a definitive statement though, so maybe we should get some feedback from @parkr (and @mnot)?

[mnot]

Please don't worry about my feelings -- I'm a total Ruby newbie! Just want the functionality.

[alfredxing]

@paulrobertlloyd Can you quickly check if, with incremental regen enabled, editing the contents of a post also changes the archive page? The archive page would have to output {{ post.content }}. That was @mnot's original issue, and why the regeneration stuff had to be added.

[paulrobertlloyd]

@alfredxing I was a little concerned when you asked me to check against this original issue, that this might invalidate my conclusion. But no! Adding item.content to archive templates and then changing item content, saw that content change. Whoop!

There should probably be some automated tests to validate this going forward, but not sure how you would (or even if you could) test for that. Can we merge this PR, or do we need to write tests for regeneration? If so, I’d need some help doing that.

[paulrobertlloyd]

Any thoughts on the above @alfredxing?

[alfredxing]

@paulrobertlloyd Sorry for the wait!

I dove a bit deeper into why this was working, and it turns out that all archive pages are always regenerated. So that's good I guess! The reason for this is that Regenerator checks if the "source file" (based on the relative path of the Archive) actually exists in the filesystem—which it doesn't.

I'll merge this in for now. If anyone has any issues, please let us know!

@jekyllbot: merge +minor