Clear the regenerator cache every time we process #3592

Merged
merged 3 commits into from Mar 22, 2015

Conversation

Projects
None yet
4 participants
@nickburlett
Contributor

nickburlett commented Mar 18, 2015

To address part of #3591, clear the regenerator's cache every time the
site is processed. This ensures that the regenerator doesn't incorrectly
believe a file hasn't changed based on stale information.

Clear the regenerator cache every time we process
To address part of #3591, clear the regenerator's cache every time the
site is processed. This ensures that the regenerator doesn't incorrectly
believe a file hasn't changed based on stale information.
@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 18, 2015

Contributor

I'm not entirely sure how to deal with the other part of #3591 (where it doesn't cache or update metadata for all dependencies), but this at least fixes the bigger problem of not regenerating files that do need regeneration.

Contributor

nickburlett commented Mar 18, 2015

I'm not entirely sure how to deal with the other part of #3591 (where it doesn't cache or update metadata for all dependencies), but this at least fixes the bigger problem of not regenerating files that do need regeneration.

@@ -59,6 +59,14 @@ def clear
@cache = {}

This comment has been minimized.

@parkr

parkr Mar 18, 2015

Member

if we go with this, this should just use that helper

@parkr

parkr Mar 18, 2015

Member

if we go with this, this should just use that helper

@@ -68,6 +68,7 @@ def reset
self.static_files = []
self.data = {}
@collections = nil
+ @regenerator.clear_cache()

This comment has been minimized.

@parkr

parkr Mar 18, 2015

Member

in ruby, you don't include the () unless it takes arguments :)

@parkr

parkr Mar 18, 2015

Member

in ruby, you don't include the () unless it takes arguments :)

This comment has been minimized.

@nickburlett

nickburlett Mar 18, 2015

Contributor

Ah, yes... can you tell it's been a while since I last coded in ruby? (^_^)
I'll fix this and regenerator clear tonight.

@nickburlett

nickburlett Mar 18, 2015

Contributor

Ah, yes... can you tell it's been a while since I last coded in ruby? (^_^)
I'll fix this and regenerator clear tonight.

This comment has been minimized.

@parkr

parkr Mar 22, 2015

Member

should this clear everything, or just the cache?

/cc @alfredxing

@parkr

parkr Mar 22, 2015

Member

should this clear everything, or just the cache?

/cc @alfredxing

This comment has been minimized.

@nickburlett

nickburlett Mar 22, 2015

Contributor

I don't think we want to clear the metadata... it should still be a correct representation of what was generated previously. It's just the cache of whether or not a path needs to be regenerated that's now stale.

@nickburlett

nickburlett Mar 22, 2015

Contributor

I don't think we want to clear the metadata... it should still be a correct representation of what was generated previously. It's just the cache of whether or not a path needs to be regenerated that's now stale.

nickburlett added some commits Mar 19, 2015

Use the new clear_cache method
Instead of assigning `@cache = {}`, clear the cache using `clear_cache`
Regenerator cache clearing tests
Two tests for the regenerator cache clearing changes:

1. Intrusively test that the regnerator.clear_cache actually clears the cache ( in test/test_regenerator.rb )
2. Test that incremental building regenerates files that have changed that previously were unchanged ( in test/test_site.rb )
@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 19, 2015

Contributor

I updated the code to use the clear_cache method instead of manually assigning @cache = {} and I've added a test that would fail without the cache clearing in site.reset

Contributor

nickburlett commented Mar 19, 2015

I updated the code to use the clear_cache method instead of manually assigning @cache = {} and I've added a test that would fail without the cache clearing in site.reset

@parkr

This comment has been minimized.

Show comment
Hide comment
Member

parkr commented Mar 20, 2015

Cool. @alfredxing?

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Mar 20, 2015

Member

👍 Thanks! I can't believe I didn't notice this myself earlier...

Member

alfredxing commented Mar 20, 2015

👍 Thanks! I can't believe I didn't notice this myself earlier...

parkr added a commit that referenced this pull request Mar 22, 2015

@parkr parkr merged commit e915270 into jekyll:master Mar 22, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Mar 22, 2015

nickburlett added a commit to nickburlett/jekyll that referenced this pull request Mar 22, 2015

nickburlett added a commit to nickburlett/jekyll that referenced this pull request Mar 24, 2015

@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.