Process metadata for all dependencies #3608

Merged
merged 1 commit into from Apr 10, 2015

Conversation

Projects
None yet
4 participants
@nickburlett
Contributor

nickburlett commented Mar 22, 2015

When adding a dependency, also add the dependency to the metadata hash.

Addresses part 1 of #3591. Prior to this fix, the regnerator only paid attention the mtime of the first dependency it checked, so for posts/pages with N multiple dependencies (i.e., every layout file used to render them), it continues to regenerate the post/page approximately N times, at which point it's seen all of the dependencies.

Process metadata for all dependencies
When adding a dependency, also add the dependency to the metadata hash.

Addresses part 1 of #3591. Prior to this fix, the regnerator only paid attention the mtime of the first dependency it checked, so for posts/pages with N multiple dependencies (i.e., every layout file used to render them), it continues to regenerate the post/page approximately N times, at which point it's seen all of the dependencies.
@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Mar 22, 2015

Member

Hmm... I feel like there's a reason I didn't add the dependencies, but I can't think of what it was. I'll look into this more.

Member

alfredxing commented Mar 22, 2015

Hmm... I feel like there's a reason I didn't add the dependencies, but I can't think of what it was. I'll look into this more.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2015

Member

Ok! Let me know if you need anything. Thanks alfred!!

Member

parkr commented Mar 24, 2015

Ok! Let me know if you need anything. Thanks alfred!!

@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Apr 7, 2015

Contributor

@alfredxing Have you had a chance to look into this?

I've been using it for a while now, and it's been helpful. I haven't been making heavy use of my jekyll project, but what work I've been able to do has been made much easier with this change. I'm using a variation on the fauno/jekyll-pandoc-multiple-formats plugin that generates PDFs from various pages on the site, and this adds several minutes to the regeneration time. With the changes in d4b8f0d it takes much less time, as it only updates the document I'm changing.

I'd love to see this merged into the official branch!

Contributor

nickburlett commented Apr 7, 2015

@alfredxing Have you had a chance to look into this?

I've been using it for a while now, and it's been helpful. I haven't been making heavy use of my jekyll project, but what work I've been able to do has been made much easier with this change. I'm using a variation on the fauno/jekyll-pandoc-multiple-formats plugin that generates PDFs from various pages on the site, and this adds several minutes to the regeneration time. With the changes in d4b8f0d it takes much less time, as it only updates the document I'm changing.

I'd love to see this merged into the official branch!

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Apr 7, 2015

Member

@nickburlett I've thought about it but haven't come up with anything. It's great to hear that it's working well for you in use, so we might as well merge this in.

@parkr Anything need to be done here before merge?

Member

alfredxing commented Apr 7, 2015

@nickburlett I've thought about it but haven't come up with anything. It's great to hear that it's working well for you in use, so we might as well merge this in.

@parkr Anything need to be done here before merge?

@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Apr 7, 2015

Contributor

@alfredxing To be fair, I've probably only used it for about ten hours of editing over the past couple of weeks, and I'm not sure if my work load is representative of what others need.

Contributor

nickburlett commented Apr 7, 2015

@alfredxing To be fair, I've probably only used it for about ten hours of editing over the past couple of weeks, and I'm not sure if my work load is representative of what others need.

- metadata[path]["deps"] << dependency unless metadata[path]["deps"].include? dependency
+ if !metadata[path]["deps"].include? dependency
+ metadata[path]["deps"] << dependency
+ add(dependency) unless metadata.include?(dependency)

This comment has been minimized.

@parkr

parkr Apr 7, 2015

Member

should add() be idempotent, @alfredxing?

@parkr

parkr Apr 7, 2015

Member

should add() be idempotent, @alfredxing?

This comment has been minimized.

@alfredxing

alfredxing Apr 10, 2015

Member

Ideally it should be. I think I ended up doing things like the unless metadata.include?(dependency) above, which isn't really good.

@alfredxing

alfredxing Apr 10, 2015

Member

Ideally it should be. I think I ended up doing things like the unless metadata.include?(dependency) above, which isn't really good.

This comment has been minimized.

@parkr

parkr Apr 10, 2015

Member

What do you mean, "ideally"? 😉

@parkr

parkr Apr 10, 2015

Member

What do you mean, "ideally"? 😉

This comment has been minimized.

@alfredxing

alfredxing Apr 10, 2015

Member

Well, it should be, but it's not a bug or anything. So, yes, we should change it, but maybe as a separate refactor/documentation issue/PR instead?

@alfredxing

alfredxing Apr 10, 2015

Member

Well, it should be, but it's not a bug or anything. So, yes, we should change it, but maybe as a separate refactor/documentation issue/PR instead?

This comment has been minimized.

@parkr

parkr Apr 10, 2015

Member

👍

@parkr

parkr Apr 10, 2015

Member

👍

@@ -104,7 +104,10 @@ def modified?(path)
def add_dependency(path, dependency)
return if (metadata[path].nil? || @disabled)
- metadata[path]["deps"] << dependency unless metadata[path]["deps"].include? dependency
+ if !metadata[path]["deps"].include? dependency

This comment has been minimized.

@parkr

parkr Apr 7, 2015

Member

maybe it should be a Set instead of just an Array? Does that slow it down unacceptably? That would eliminate the need for the check here.

@parkr

parkr Apr 7, 2015

Member

maybe it should be a Set instead of just an Array? Does that slow it down unacceptably? That would eliminate the need for the check here.

This comment has been minimized.

@alfredxing

alfredxing Apr 10, 2015

Member

A set sounds good. Wouldn't it actually speed it up, since it's constant time find (instead of linear time)?

@alfredxing

alfredxing Apr 10, 2015

Member

A set sounds good. Wouldn't it actually speed it up, since it's constant time find (instead of linear time)?

This comment has been minimized.

@parkr

parkr Apr 10, 2015

Member

I'm honestly not sure.

@parkr

parkr Apr 10, 2015

Member

I'm honestly not sure.

parkr added a commit that referenced this pull request Apr 10, 2015

@parkr parkr merged commit bf98004 into jekyll:master Apr 10, 2015

1 check passed

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

parkr added a commit that referenced this pull request Apr 10, 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.