prototype of jekyll hooks, encapsulated #3553

Merged
merged 4 commits into from May 10, 2015

Conversation

Projects
None yet
10 participants
@stevecrozz
Contributor

stevecrozz commented Mar 7, 2015

This is a second attempt at #3498 (adding hooks to Jekyll). In my last attempt @envygeeks had some complaints about the lack of encapsulation. This PR is an attempt to address those concerns and try out a new idea. This is definitely not ready to merge, but I wanted to get some eyes on it before I implement and write tests for the whole thing. The hooks concept is inspired by, but in this instance not perfectly compatible with octopress-hooks, so it's a bit of a departure from #3392 by @parkr, but I think it nicely encapsulates the hook logic.

Here's a practical example of how you could use one of these hooks to do something useful: https://github.com/stevecrozz/featherblade

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 8, 2015

Member

Don't worry about the circleci failure.

This looks pretty good! @envygeeks?

Member

parkr commented Mar 8, 2015

Don't worry about the circleci failure.

This looks pretty good! @envygeeks?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 8, 2015

Contributor

👍 I like it!

Contributor

envygeeks commented Mar 8, 2015

👍 I like it!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 8, 2015

Contributor

Sorry about the 👎 I actually meant to do 👍

Contributor

envygeeks commented Mar 8, 2015

Sorry about the 👎 I actually meant to do 👍

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 8, 2015

Contributor

Great. I'll flesh out the rest of it and add some test coverage.

Contributor

stevecrozz commented Mar 8, 2015

Great. I'll flesh out the rest of it and add some test coverage.

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 8, 2015

Contributor

This is a bit more fleshed out now, although still lacking in tests and docs. I did do some one-off tests and it all seems to be working pretty well. Every hook receives the object being "hooked into" as the first argument.

One major difference from octopress-hooks worth pointing out is how I'm dealing with the payload objects just before rendering.

In octopress-hooks, there's a special hook for merging in new payload params. But I took a different approach and just pass the payload as an argument to each of the pre_render hooks. Anyone who implements a pre_render hook will receive the payload object and can modify it there if necessary as well as do any other things that need doing before rendering. pre_render is the only hook that receives a second argument at the moment. @imathis: let me know if you have any thoughts on this, especially if you already rejected this idea for some reason.

Next up is tests and docs, but I'll give this PR a few days to bake in case there's more feedback.

Contributor

stevecrozz commented Mar 8, 2015

This is a bit more fleshed out now, although still lacking in tests and docs. I did do some one-off tests and it all seems to be working pretty well. Every hook receives the object being "hooked into" as the first argument.

One major difference from octopress-hooks worth pointing out is how I'm dealing with the payload objects just before rendering.

In octopress-hooks, there's a special hook for merging in new payload params. But I took a different approach and just pass the payload as an argument to each of the pre_render hooks. Anyone who implements a pre_render hook will receive the payload object and can modify it there if necessary as well as do any other things that need doing before rendering. pre_render is the only hook that receives a second argument at the moment. @imathis: let me know if you have any thoughts on this, especially if you already rejected this idea for some reason.

Next up is tests and docs, but I'll give this PR a few days to bake in case there's more feedback.

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 14, 2015

Contributor

I'm working on cucumber tests now. How thorough do these tests need to be? Do we want each hook for every container to have its own test scenario?

Contributor

stevecrozz commented Mar 14, 2015

I'm working on cucumber tests now. How thorough do these tests need to be? Do we want each hook for every container to have its own test scenario?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 14, 2015

Contributor

I say just write them and if there is a problem we'll point it out!

Contributor

envygeeks commented Mar 14, 2015

I say just write them and if there is a problem we'll point it out!

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 14, 2015

Contributor

@envygeeks / @parkr I've got cucumber tests and docs written. Do you have any feedback?

Contributor

stevecrozz commented Mar 14, 2015

@envygeeks / @parkr I've got cucumber tests and docs written. Do you have any feedback?

@@ -469,6 +469,179 @@ module Jekyll
end
{% endhighlight %}
+## Hooks
+

This comment has been minimized.

@parkr

parkr Mar 16, 2015

Member

Can you add an "unreleased" note here? Check out #3556 for an example.

@parkr

parkr Mar 16, 2015

Member

Can you add an "unreleased" note here? Check out #3556 for an example.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 16, 2015

Member

Unless you have some thoughts, @envygeeks, let's merge and get this out there in a new beta so people can start building for it. Best way to fix bugs is through actual use of the platform.

Member

parkr commented Mar 16, 2015

Unless you have some thoughts, @envygeeks, let's merge and get this out there in a new beta so people can start building for it. Best way to fix bugs is through actual use of the platform.

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 16, 2015

Contributor

Amended commit to include "unreleased" note in the docs

Contributor

stevecrozz commented Mar 16, 2015

Amended commit to include "unreleased" note in the docs

@parkr parkr referenced this pull request Mar 16, 2015

Closed

3.0 RELEASE GAMEPLAN #3324

7 of 7 tasks complete

@parkr parkr added this to the 3.0 milestone Mar 16, 2015

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Mar 17, 2015

Member

👍 Nice work on this! We're missing hooks for Documents but that shouldn't be a huge issue.

Member

alfredxing commented Mar 17, 2015

👍 Nice work on this! We're missing hooks for Documents but that shouldn't be a huge issue.

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Mar 17, 2015

Contributor

I'm sorry I'm a bit late to this.

Unless I've overlooked it, this does not include:

  • Site: pre_read, merge_payload
  • Page/Post: merge_payload

For reference here's Octopress's hook timeline.

Contributor

imathis commented Mar 17, 2015

I'm sorry I'm a bit late to this.

Unless I've overlooked it, this does not include:

  • Site: pre_read, merge_payload
  • Page/Post: merge_payload

For reference here's Octopress's hook timeline.

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 18, 2015

Contributor

@imathis That's correct. I removed pre_read from my PR while writing the test cases because I couldn't come up with a use case to test that wasn't already covered. There may be some good ones, but I didn't think of any.

As for merge_payload, I did depart from the octopress-hooks approach which is why I was interested in your feedback. I tried to cover the use case of altering the payload using pre_render which passes the payload to the callback as its second parameter. There's less code and less to test in this approach and it seems more flexible, but I'd be interested to know if I missed a good reason to implement it the way you already did.

Contributor

stevecrozz commented Mar 18, 2015

@imathis That's correct. I removed pre_read from my PR while writing the test cases because I couldn't come up with a use case to test that wasn't already covered. There may be some good ones, but I didn't think of any.

As for merge_payload, I did depart from the octopress-hooks approach which is why I was interested in your feedback. I tried to cover the use case of altering the payload using pre_render which passes the payload to the callback as its second parameter. There's less code and less to test in this approach and it seems more flexible, but I'd be interested to know if I missed a good reason to implement it the way you already did.

@parkr

This comment has been minimized.

Show comment
Hide comment
Member

parkr commented Mar 30, 2015

@imathis ^^?

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Mar 31, 2015

Contributor

I'm going to create a branch of octopress-hooks that translates hooks to be based on this. If my tests pass. 👍 Let's see how long this takes :)

Contributor

imathis commented Mar 31, 2015

I'm going to create a branch of octopress-hooks that translates hooks to be based on this. If my tests pass. 👍 Let's see how long this takes :)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 31, 2015

Contributor

Can we focus more on what we need and less on what we have?

Contributor

envygeeks commented Mar 31, 2015

Can we focus more on what we need and less on what we have?

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Mar 31, 2015

Contributor

@envygeeks I'm focusing on what I need. I built the features in Octopress hooks because I needed them. If I can keep that functionality, I'll be good to go. Otherwise I'll have to keep monkey patching my own hooks.

Contributor

imathis commented Mar 31, 2015

@envygeeks I'm focusing on what I need. I built the features in Octopress hooks because I needed them. If I can keep that functionality, I'll be good to go. Otherwise I'll have to keep monkey patching my own hooks.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 31, 2015

Contributor

@imathis Well if you run into trouble then please do post the hooks that are failing and where they originate and are used at so I might be able to help you two come to compromise by possibly reworking both sides to see if we can fit your hooks into his hook system or if we need to truly have him change his hook system.

Contributor

envygeeks commented Mar 31, 2015

@imathis Well if you run into trouble then please do post the hooks that are failing and where they originate and are used at so I might be able to help you two come to compromise by possibly reworking both sides to see if we can fit your hooks into his hook system or if we need to truly have him change his hook system.

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Mar 31, 2015

Contributor

@envygeeks Thanks. I don't anticipate having to rework this system. I think most of the stuff I'm doing should map to these pretty well. I sure like this code better though. I do have one question though. The way I originally wrote the hooks, they were extended from Jekyll::Plugin. This meant I could set priority for each hook, a feature I've found pretty useful for some of the plugins I've written. As far as I can tell, I'd set the priority on the plugin. If I needed different priority hooks, I'd just have to create separate plugins for them.

Contributor

imathis commented Mar 31, 2015

@envygeeks Thanks. I don't anticipate having to rework this system. I think most of the stuff I'm doing should map to these pretty well. I sure like this code better though. I do have one question though. The way I originally wrote the hooks, they were extended from Jekyll::Plugin. This meant I could set priority for each hook, a feature I've found pretty useful for some of the plugins I've written. As far as I can tell, I'd set the priority on the plugin. If I needed different priority hooks, I'd just have to create separate plugins for them.

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Mar 31, 2015

Contributor

So far this is working beautifully. I've had one issue, though. After I set up a test for page hooks, I discovered that the incremental regeneration cache was preventing the hook from affecting the page. I cleared the cache and then it worked. I have a feeling that, "Have you tried deleting your Jekyll cache?" may become a common troubleshooting question.

Contributor

imathis commented Mar 31, 2015

So far this is working beautifully. I've had one issue, though. After I set up a test for page hooks, I discovered that the incremental regeneration cache was preventing the hook from affecting the page. I cleared the cache and then it worked. I have a feeling that, "Have you tried deleting your Jekyll cache?" may become a common troubleshooting question.

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Mar 31, 2015

Contributor

I've rewritten my Octopress Hooks tests to use this instead of Octopress Hooks and it's awesome. The two things I lack, which I'd really like are:

  1. Collection hooks.
  2. All hooks (a way to write a hook that runs on any page type (Page, Post, Collection)

I definitely think Collection hooks are necessary, "All Hooks" are somewhat less necessary, but in practice the ability to tell Jekyll to process all content, avoids duplication and reduces bugs in plugins. I'm not sure how you'd implement it, but it would be really useful.

Thanks so much for this. It's way nicer to use than Octopress Hooks. 👍

Contributor

imathis commented Mar 31, 2015

I've rewritten my Octopress Hooks tests to use this instead of Octopress Hooks and it's awesome. The two things I lack, which I'd really like are:

  1. Collection hooks.
  2. All hooks (a way to write a hook that runs on any page type (Page, Post, Collection)

I definitely think Collection hooks are necessary, "All Hooks" are somewhat less necessary, but in practice the ability to tell Jekyll to process all content, avoids duplication and reduces bugs in plugins. I'm not sure how you'd implement it, but it would be really useful.

Thanks so much for this. It's way nicer to use than Octopress Hooks. 👍

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Mar 31, 2015

Member

Incremental regeneration doesn't render or write pages/posts if they aren't changed. We can move hooks like post_render and post_write so they get triggered, or add new hooks that get called even when posts/pages are not rendered.

Member

alfredxing commented Mar 31, 2015

Incremental regeneration doesn't render or write pages/posts if they aren't changed. We can move hooks like post_render and post_write so they get triggered, or add new hooks that get called even when posts/pages are not rendered.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 31, 2015

Member

"All Hooks" are somewhat less necessary, but in practice the ability to tell Jekyll to process all content, avoids duplication and reduces bugs in plugins

Eventually Document will be the main class; Page and Post will no longer exist at that point. Adding a hook to Document will take care of this. What will be harder, is getting more specific. How do you attach a filter to just one collection? Have the method only run if the doc's collection is X? Perhaps that will work for now.

Member

parkr commented Mar 31, 2015

"All Hooks" are somewhat less necessary, but in practice the ability to tell Jekyll to process all content, avoids duplication and reduces bugs in plugins

Eventually Document will be the main class; Page and Post will no longer exist at that point. Adding a hook to Document will take care of this. What will be harder, is getting more specific. How do you attach a filter to just one collection? Have the method only run if the doc's collection is X? Perhaps that will work for now.

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 31, 2015

Contributor

@imathis : @envygeeks and I were just talking about the priority idea. In order to do it the way that seems to make the most sense to us, we'll need a way to apply a name to a hook. So we have some changes to this PR in mind to support this. Roughly, hooks would go back to being classes, as they are in octopress-hooks, and they would implement a 'run' method and have a name (the name being the name of the class):

class MyHook
  def run(page); end
end

class MyOtherHook
  def run(page); end
end

Jekyll::Hooks.register Jekyll::Page, :post_render, MyHook

# insert one hook before another, hook specified as a symbol here in case MyHook
# is not yet defined (due to not yet having been loaded)
Jekyll::Hooks.register Jekyll::Page, :post_render, MyOtherHook, before: :MyHook

I agreed to start working on this idea. As always I do appreciate feedback on this. Let me know if this looks good or bad to any of you.

@imathis I have no objections to collection hooks. If you can contribute to the cucumber tests / implementation / docs here I think we can include those.

Contributor

stevecrozz commented Mar 31, 2015

@imathis : @envygeeks and I were just talking about the priority idea. In order to do it the way that seems to make the most sense to us, we'll need a way to apply a name to a hook. So we have some changes to this PR in mind to support this. Roughly, hooks would go back to being classes, as they are in octopress-hooks, and they would implement a 'run' method and have a name (the name being the name of the class):

class MyHook
  def run(page); end
end

class MyOtherHook
  def run(page); end
end

Jekyll::Hooks.register Jekyll::Page, :post_render, MyHook

# insert one hook before another, hook specified as a symbol here in case MyHook
# is not yet defined (due to not yet having been loaded)
Jekyll::Hooks.register Jekyll::Page, :post_render, MyOtherHook, before: :MyHook

I agreed to start working on this idea. As always I do appreciate feedback on this. Let me know if this looks good or bad to any of you.

@imathis I have no objections to collection hooks. If you can contribute to the cucumber tests / implementation / docs here I think we can include those.

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Mar 31, 2015

Contributor

I'd be happy to contribute the Collections stuff.

On the priority, I'm looking for a way of nudging global priority like how Jekyll Plugins have the :highest, :high, :normal, :low, :lowest. I use this for octopress-paginate to ensure that other hooks have run their transformations before the paginate plugin merges the page's payload with the pagination values.

I use them again in octopress-multilingual where I try to merge page and site payloads before other hooks, so that other hooks can work with multilingual features.

I'm sure users could manually set priority by setting the load order of their gems, but it would be ideal if people didn't have to think about load order in order to use a plugin, but that would be the fallback strategy if plugins had competing priority which caused a conflict. Additionally being able to set priority like this allows user plugins written in the _plugins directory to participate in the priority chain.

Do you think this will take a lot of effort to do? Does it seem reasonable?

Contributor

imathis commented Mar 31, 2015

I'd be happy to contribute the Collections stuff.

On the priority, I'm looking for a way of nudging global priority like how Jekyll Plugins have the :highest, :high, :normal, :low, :lowest. I use this for octopress-paginate to ensure that other hooks have run their transformations before the paginate plugin merges the page's payload with the pagination values.

I use them again in octopress-multilingual where I try to merge page and site payloads before other hooks, so that other hooks can work with multilingual features.

I'm sure users could manually set priority by setting the load order of their gems, but it would be ideal if people didn't have to think about load order in order to use a plugin, but that would be the fallback strategy if plugins had competing priority which caused a conflict. Additionally being able to set priority like this allows user plugins written in the _plugins directory to participate in the priority chain.

Do you think this will take a lot of effort to do? Does it seem reasonable?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 31, 2015

Contributor

Doing it your way would drastically simplify the way the code has to work compared to the way we discussed last night where we have named hook points and a before or after, if we just have a numbered priority set then sure it's tons easier to select on.

Contributor

envygeeks commented Mar 31, 2015

Doing it your way would drastically simplify the way the code has to work compared to the way we discussed last night where we have named hook points and a before or after, if we just have a numbered priority set then sure it's tons easier to select on.

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 31, 2015

Contributor

It would certainly be an easier change if hooks can remain as plain Ruby callables and we don't wrap them in classes. Should I just add a 'priority' keyword argument to Jekyll::Hooks::register?

Contributor

stevecrozz commented Mar 31, 2015

It would certainly be an easier change if hooks can remain as plain Ruby callables and we don't wrap them in classes. Should I just add a 'priority' keyword argument to Jekyll::Hooks::register?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 31, 2015

Contributor

@stevecrozz I think so, easier to do, less work and suites his needs without breaking much!

Contributor

envygeeks commented Mar 31, 2015

@stevecrozz I think so, easier to do, less work and suites his needs without breaking much!

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Mar 31, 2015

Contributor

One more thing. I'm not certain the names :highest, :high, :normal, :low, :lowest are clear because I can't tell if high priority means it should run earlier or later than the others. I have another suggestion which is :earliest, :early, :normal, :late, :latest. Any thoughts on these? Am I alone in my confusion?

Contributor

stevecrozz commented Mar 31, 2015

One more thing. I'm not certain the names :highest, :high, :normal, :low, :lowest are clear because I can't tell if high priority means it should run earlier or later than the others. I have another suggestion which is :earliest, :early, :normal, :late, :latest. Any thoughts on these? Am I alone in my confusion?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 31, 2015

Member

The problem with all of this is really that you can't know the way the hooks will be run, e.g. what if two plugins have :earliest or :highest? Which one is run first?

Maybe we should order them more systematically, like with ints or something.

Member

parkr commented Mar 31, 2015

The problem with all of this is really that you can't know the way the hooks will be run, e.g. what if two plugins have :earliest or :highest? Which one is run first?

Maybe we should order them more systematically, like with ints or something.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 31, 2015

Contributor

We already had an elaborate plan for not only out-of-order plugin priority but generic plugin priority which was what @stevecrozz mentioned yesterday, if what we want is the ability to insert yourself before another hook then the plan from yesterday is better than the plan from today.

Contributor

envygeeks commented Mar 31, 2015

We already had an elaborate plan for not only out-of-order plugin priority but generic plugin priority which was what @stevecrozz mentioned yesterday, if what we want is the ability to insert yourself before another hook then the plan from yesterday is better than the plan from today.

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Apr 1, 2015

Contributor

@parkr: I'm fine with integer ranking or symbols. The Jekyll priorities are currently number based, but use symbols as human friendly labels.

@envygeeks: Thinking about it more, I don't see your solution from yesterday working out very well. In the case of my multilingual plugin, I really do want it to run before most plugins, setting it to :high priority ensures that I can do that. This means that other plugins don't have to be aware of multilingual features to work properly. It would be unfortunate if all other plugins had to opt-in by registering their hooks to run after octopress-multilingual.And of course it would be impossible for me to know which plugin should be run after and keep updating my plugin, adding them to the list of hooks that should be registered after.

I prefer an agnostic global order approach.

Contributor

imathis commented Apr 1, 2015

@parkr: I'm fine with integer ranking or symbols. The Jekyll priorities are currently number based, but use symbols as human friendly labels.

@envygeeks: Thinking about it more, I don't see your solution from yesterday working out very well. In the case of my multilingual plugin, I really do want it to run before most plugins, setting it to :high priority ensures that I can do that. This means that other plugins don't have to be aware of multilingual features to work properly. It would be unfortunate if all other plugins had to opt-in by registering their hooks to run after octopress-multilingual.And of course it would be impossible for me to know which plugin should be run after and keep updating my plugin, adding them to the list of hooks that should be registered after.

I prefer an agnostic global order approach.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 1, 2015

Contributor

@imathis your multilingual plugin shouldn't be a hook, it should be a generator tbh.

Contributor

envygeeks commented Apr 1, 2015

@imathis your multilingual plugin shouldn't be a hook, it should be a generator tbh.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 1, 2015

Contributor

Or, if it's modifying things like the nav, why does it need to run first or before everything? I would assume that as translator it would want to run absolutely last so that it has access to the final modified source no?

Contributor

envygeeks commented Apr 1, 2015

Or, if it's modifying things like the nav, why does it need to run first or before everything? I would assume that as translator it would want to run absolutely last so that it has access to the final modified source no?

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Apr 1, 2015

Contributor

@envygeeks Perhaps it could be done with a generator, I don't know. Will a generator let me merge the payload of the site and all the pages? As far as I understand, I need hooks for that. But this is an aside.

What do you think about a global priority? I know it's more of a club than a scalpel, but it also seems they could coexist. People could use the simple global priority system, or if they know of another plugin's hooks they want to proceed or follow after, they could use that instead. This way hooks could be shipped with a simple priority system, but as validating use-cases arise, your more advanced priority system could be added.

Contributor

imathis commented Apr 1, 2015

@envygeeks Perhaps it could be done with a generator, I don't know. Will a generator let me merge the payload of the site and all the pages? As far as I understand, I need hooks for that. But this is an aside.

What do you think about a global priority? I know it's more of a club than a scalpel, but it also seems they could coexist. People could use the simple global priority system, or if they know of another plugin's hooks they want to proceed or follow after, they could use that instead. This way hooks could be shipped with a simple priority system, but as validating use-cases arise, your more advanced priority system could be added.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 1, 2015

Contributor

I wouldn't think of it as a global priority but in that scenario I would vote for it to just be :top or :bottom, you either insert yourself at the top of the list or you insert yourself at the bottom of the list if you don't want to hook before or after a specific plugin (with the default being :bottom) that would have already been the case to begin with (always inserting at the bottom) but inserting at the top might require an extra conditional on the sort but that's no big deal.

Contributor

envygeeks commented Apr 1, 2015

I wouldn't think of it as a global priority but in that scenario I would vote for it to just be :top or :bottom, you either insert yourself at the top of the list or you insert yourself at the bottom of the list if you don't want to hook before or after a specific plugin (with the default being :bottom) that would have already been the case to begin with (always inserting at the bottom) but inserting at the top might require an extra conditional on the sort but that's no big deal.

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Apr 2, 2015

Contributor

I'm fine with a three level priority system. :top, :normal, :bottom. I think it's useful to be able to indicate a higher priority as well as a lower priority than normal.

Contributor

imathis commented Apr 2, 2015

I'm fine with a three level priority system. :top, :normal, :bottom. I think it's useful to be able to indicate a higher priority as well as a lower priority than normal.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 29, 2015

Contributor

Leaving aside you want to unseed in a one time run.... unless you have absolute control over every instance of the hook system you can't guarantee anything so your logic is broken to begin with. You cannot ensure that every hook author will not just use :highest, so there is no point in even including it because as you just pointed out, every Jim Bob programmer is going to just use :highest to "unseed" content making 5 levels moot to begin with.

Contributor

envygeeks commented Apr 29, 2015

Leaving aside you want to unseed in a one time run.... unless you have absolute control over every instance of the hook system you can't guarantee anything so your logic is broken to begin with. You cannot ensure that every hook author will not just use :highest, so there is no point in even including it because as you just pointed out, every Jim Bob programmer is going to just use :highest to "unseed" content making 5 levels moot to begin with.

@StevenBlack

This comment has been minimized.

Show comment
Hide comment
@StevenBlack

StevenBlack Apr 29, 2015

@envygeeks I recommend you start here and then maybe we can talk about "broken logic" with some sort of sensible basis for evaluation for the design of hookable software.

@envygeeks I recommend you start here and then maybe we can talk about "broken logic" with some sort of sensible basis for evaluation for the design of hookable software.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 29, 2015

Member

@StevenBlack Please don't be snarky. @envygeeks is a very good developer.

I agree that the wording could be better. :first, :unspecified, and :last might make even more sense if we go with 3 levels. What do you think?

Member

parkr commented Apr 29, 2015

@StevenBlack Please don't be snarky. @envygeeks is a very good developer.

I agree that the wording could be better. :first, :unspecified, and :last might make even more sense if we go with 3 levels. What do you think?

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Apr 29, 2015

Contributor

@parkr, I'm fine with those labels. For this other stuff, I'm staying out of it. 🌴

Contributor

imathis commented Apr 29, 2015

@parkr, I'm fine with those labels. For this other stuff, I'm staying out of it. 🌴

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 29, 2015

Contributor

@StevenBlack I've reviewed your code, I don't think you should be handing out advice any time soon.

@parkr @imathis my only concern is the way people tend to always behave, :highest will probably be the highest common usage because everybody wants to be important so if we end up with everybody wanting to be the most important why not just reduce the levels and you can either choose to be not-important, don't-care or just plain old important. I like the way you labled those new labels.

Contributor

envygeeks commented Apr 29, 2015

@StevenBlack I've reviewed your code, I don't think you should be handing out advice any time soon.

@parkr @imathis my only concern is the way people tend to always behave, :highest will probably be the highest common usage because everybody wants to be important so if we end up with everybody wanting to be the most important why not just reduce the levels and you can either choose to be not-important, don't-care or just plain old important. I like the way you labled those new labels.

@StevenBlack

This comment has been minimized.

Show comment
Hide comment
@StevenBlack

StevenBlack Apr 29, 2015

@envygeeks for the record, I wrote my first-ever .rb file 11-days ago, first commit 9-days ago.

I can tell you that extensibility in general, and hook operations in particular, are easy to get-wrong.

@envygeeks for the record, I wrote my first-ever .rb file 11-days ago, first commit 9-days ago.

I can tell you that extensibility in general, and hook operations in particular, are easy to get-wrong.

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis Apr 29, 2015

Contributor

If you two want to fight, meet at the flag pole after school but keep this kind of thing out of issues.

Contributor

imathis commented Apr 29, 2015

If you two want to fight, meet at the flag pole after school but keep this kind of thing out of issues.

stevecrozz added some commits Mar 7, 2015

Refine hook implementation
- hooks are registered to symbol owners rather than classes directly
- during registration, add the ability to specify owner as an array to
  register the same hook to multiple owners
- add optional priority during registration as a symbol (:low, :normal,
  :high)
- implement hooks for collections as they are in octopress-hooks, aside
  from post_init
@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz May 2, 2015

Contributor

This should address all the feedback @imathis. Just in time for JekyllConf!

Contributor

stevecrozz commented May 2, 2015

This should address all the feedback @imathis. Just in time for JekyllConf!

lib/jekyll/document.rb
@@ -175,11 +175,15 @@ def destination(base_directory)
#
# Returns nothing.
def write(dest)
+ Jekyll::Hooks.trigger self, :post_render

This comment has been minimized.

@parkr

parkr May 2, 2015

Member

Why not put this at the end of render?

@parkr

parkr May 2, 2015

Member

Why not put this at the end of render?

This comment has been minimized.

@stevecrozz

stevecrozz May 2, 2015

Contributor

That's fine with me. It's here just to match the behavior of octopress hooks:
https://github.com/octopress/hooks/blob/master/lib/octopress-hooks.rb#L256

I'm fine with moving it though. Do you still want me to do that?

@stevecrozz

stevecrozz May 2, 2015

Contributor

That's fine with me. It's here just to match the behavior of octopress hooks:
https://github.com/octopress/hooks/blob/master/lib/octopress-hooks.rb#L256

I'm fine with moving it though. Do you still want me to do that?

This comment has been minimized.

@parkr

parkr May 3, 2015

Member

Yeah, I think it'd make more sense. :)

@parkr

parkr May 3, 2015

Member

Yeah, I think it'd make more sense. :)

lib/jekyll/hooks.rb
+ NotAvailable = Class.new(RuntimeError)
+
+ # register hook(s) to be called later
+ def self.register(owners, event, priority: nil, &block)

This comment has been minimized.

@parkr

parkr May 2, 2015

Member

We don't use named parameters anywhere else in the code base. Advantages of this are that you can specify the block without specifying the priority?

@parkr

parkr May 2, 2015

Member

We don't use named parameters anywhere else in the code base. Advantages of this are that you can specify the block without specifying the priority?

This comment has been minimized.

@envygeeks

envygeeks May 2, 2015

Contributor

I personally don't like this usage of keyword args. Why not have it default the value to the internal API's instead of having nil seep through and then deciding on the outside, have the outside default and the internal API force a value or collapse. That's just my opinion: the public API's set the usage and defaults.

@envygeeks

envygeeks May 2, 2015

Contributor

I personally don't like this usage of keyword args. Why not have it default the value to the internal API's instead of having nil seep through and then deciding on the outside, have the outside default and the internal API force a value or collapse. That's just my opinion: the public API's set the usage and defaults.

This comment has been minimized.

@envygeeks

envygeeks May 2, 2015

Contributor

Let me add, I'm not saying remove the **kwd I'm saying, lets build an API where the defaults surface.

@envygeeks

envygeeks May 2, 2015

Contributor

Let me add, I'm not saying remove the **kwd I'm saying, lets build an API where the defaults surface.

lib/jekyll/hooks.rb
+ # register hook(s) to be called later
+ def self.register(owners, event, priority: nil, &block)
+ Array(owners).each do |owner|
+ register_one(owner, event, priority: priority_value(priority), &block)

This comment has been minimized.

@parkr

parkr May 2, 2015

Member

What if block is nil?

@parkr

parkr May 2, 2015

Member

What if block is nil?

This comment has been minimized.

@stevecrozz

stevecrozz May 2, 2015

Contributor

Adding an exception when block does not resond to 'call'

@stevecrozz

stevecrozz May 2, 2015

Contributor

Adding an exception when block does not resond to 'call'

+ Jekyll::Page => :page,
+ Jekyll::Post => :post,
+ Jekyll::Document => :document,
+ }.freeze

This comment has been minimized.

@parkr

parkr May 2, 2015

Member

What if I want to make a hook for my custom plugin? Like post_sass or something?

@parkr

parkr May 2, 2015

Member

What if I want to make a hook for my custom plugin? Like post_sass or something?

This comment has been minimized.

@stevecrozz

stevecrozz May 2, 2015

Contributor

Good idea. I think it's safe to proceed with this frozen constant for now because its just an internal API. If we need a public API for adding hook points we can change this storage mechanism easily enough.

@stevecrozz

stevecrozz May 2, 2015

Contributor

Good idea. I think it's safe to proceed with this frozen constant for now because its just an internal API. If we need a public API for adding hook points we can change this storage mechanism easily enough.

@@ -158,7 +164,6 @@ def render
end
end
- payload = site_payload

This comment has been minimized.

@parkr

parkr May 2, 2015

Member

This line is non-negotiable. Totally changes the speed of regen. For one site, it went from 300 seconds to 80 seconds as a result of this line. I'd like to keep it at all costs or cache the site payload on first call.

@parkr

parkr May 2, 2015

Member

This line is non-negotiable. Totally changes the speed of regen. For one site, it went from 300 seconds to 80 seconds as a result of this line. I'd like to keep it at all costs or cache the site payload on first call.

This comment has been minimized.

@stevecrozz

stevecrozz May 2, 2015

Contributor

Not sure I understand this. The same line: payload = site_payload is just 10 lines above. If it does really need to be here, do you have another suggestion on how we can allow the :site, :pre_render hook to modify the payload?

@stevecrozz

stevecrozz May 2, 2015

Contributor

Not sure I understand this. The same line: payload = site_payload is just 10 lines above. If it does really need to be here, do you have another suggestion on how we can allow the :site, :pre_render hook to modify the payload?

This comment has been minimized.

@stevecrozz

stevecrozz May 2, 2015

Contributor

@parkr: I probably need to clarify the problem a bit more. I removed this line because it overwrites the value of the local variable 'payload' which can be altered by site pre_render hooks. There's a bunch of possible solutions to this problem but in order to suggest one, I think I need to understand this performance issue.

@stevecrozz

stevecrozz May 2, 2015

Contributor

@parkr: I probably need to clarify the problem a bit more. I removed this line because it overwrites the value of the local variable 'payload' which can be altered by site pre_render hooks. There's a bunch of possible solutions to this problem but in order to suggest one, I think I need to understand this performance issue.

This comment has been minimized.

@parkr

parkr May 3, 2015

Member

Hm. I was doing this to update the contents of the payload. If it still works as expected (they're all pointers I guess) then it should be fine. I'm sorry to be cross – I thought you removed the caching! Thanks, Steve.

@parkr

parkr May 3, 2015

Member

Hm. I was doing this to update the contents of the payload. If it still works as expected (they're all pointers I guess) then it should be fine. I'm sorry to be cross – I thought you removed the caching! Thanks, Steve.

This comment has been minimized.

@stevecrozz

stevecrozz May 3, 2015

Contributor

No prob. I just don't want to accidentally introduce a big performance issue

@stevecrozz

stevecrozz May 3, 2015

Contributor

No prob. I just don't want to accidentally introduce a big performance issue

This comment has been minimized.

@lgp36018

lgp36018 May 4, 2015

hi

Stephen Crosby notifications@github.com wrote:

@@ -158,7 +164,6 @@ def render
end
end

  •  payload = site_payload
    

No prob. I just don't want to accidentally introduce a big performance
issue


Reply to this email directly or view it on GitHub:
https://github.com/jekyll/jekyll/pull/3553/files#r29559695

Sent from my Android device with Gmail Plus. Please excuse my brevity.

@lgp36018

lgp36018 May 4, 2015

hi

Stephen Crosby notifications@github.com wrote:

@@ -158,7 +164,6 @@ def render
end
end

  •  payload = site_payload
    

No prob. I just don't want to accidentally introduce a big performance
issue


Reply to this email directly or view it on GitHub:
https://github.com/jekyll/jekyll/pull/3553/files#r29559695

Sent from my Android device with Gmail Plus. Please excuse my brevity.

site/_docs/plugins.md
+ <p><code>:site</code></p>
+ </td>
+ <td>
+ <p><code>:reset</code></p>

This comment has been minimized.

@parkr

parkr May 2, 2015

Member

After_reset?

@parkr

parkr May 2, 2015

Member

After_reset?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 2, 2015

Member

Thanks for all your work on this, @stevecrozz!!

Any other thoughts, @jekyll/core and @imathis?

Member

parkr commented May 2, 2015

Thanks for all your work on this, @stevecrozz!!

Any other thoughts, @jekyll/core and @imathis?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 2, 2015

Contributor

My only complaint is that defaults aren't on the surface API, we are seeping nil through the stack into internal API's and then setting defaults. I think that's obscure design and we should surface every default we have on the layer that is public making everything obvious.

Contributor

envygeeks commented May 2, 2015

My only complaint is that defaults aren't on the surface API, we are seeping nil through the stack into internal API's and then setting defaults. I think that's obscure design and we should surface every default we have on the layer that is public making everything obvious.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 2, 2015

Member

I would tend to agree, Jordon! Which defaults, specifically?

Member

parkr commented May 2, 2015

I would tend to agree, Jordon! Which defaults, specifically?

address code review feedback from #3553
- change site:reset to site:after_reset
- raise an exception when registering uncallable hook
- set default hook priority at the public API level
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 3, 2015

Contributor

@parkr it was originally on line 53 of hooks.rb but it looks @stevecrozz caught what I meant and correct it so 🎉 I'm liking this so far!

Contributor

envygeeks commented May 3, 2015

@parkr it was originally on line 53 of hooks.rb but it looks @stevecrozz caught what I meant and correct it so 🎉 I'm liking this so far!

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis May 3, 2015

Contributor

Lookin' good! I'll give another go at rebuilding some plugins based on this.

Contributor

imathis commented May 3, 2015

Lookin' good! I'll give another go at rebuilding some plugins based on this.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 3, 2015

Member

@gjtorikian: would you mind trying out this branch on a big site you have access to? Interested in performance change, if any. ❤️

Member

parkr commented May 3, 2015

@gjtorikian: would you mind trying out this branch on a big site you have access to? Interested in performance change, if any. ❤️

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz May 3, 2015

Contributor

@imathis You could potentially author a shim to make your current octopress-hooks based plugins work with these new hooks. Here's a totally untested start at something like that.
https://gist.github.com/stevecrozz/ca294104609efd8205bb

Contributor

stevecrozz commented May 3, 2015

@imathis You could potentially author a shim to make your current octopress-hooks based plugins work with these new hooks. Here's a totally untested start at something like that.
https://gist.github.com/stevecrozz/ca294104609efd8205bb

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis May 4, 2015

Contributor

@stevecrozz I'll give it a try, thanks for the guidance.

Contributor

imathis commented May 4, 2015

@stevecrozz I'll give it a try, thanks for the guidance.

@gjtorikian

This comment has been minimized.

Show comment
Hide comment
@gjtorikian

gjtorikian May 4, 2015

Member

would you mind trying out this branch on a big site you have access to? Interested in performance change, if any.

I kept trying it out and thought it would be a quick discovery, but kept running into problem. First, most plugins aren't Jekyll 3 compatible yet; I've logged a few issues for them. But even with them commented out, I discovered so of my own plugins/hacks are incompatible, too. It'll take me some time to upgrade those, in order to be able to test performance on Jekyll 3/this branch.

Member

gjtorikian commented May 4, 2015

would you mind trying out this branch on a big site you have access to? Interested in performance change, if any.

I kept trying it out and thought it would be a quick discovery, but kept running into problem. First, most plugins aren't Jekyll 3 compatible yet; I've logged a few issues for them. But even with them commented out, I discovered so of my own plugins/hacks are incompatible, too. It'll take me some time to upgrade those, in order to be able to test performance on Jekyll 3/this branch.

@StevenBlack

This comment has been minimized.

Show comment
Hide comment
@StevenBlack

StevenBlack May 6, 2015

it's post_read, not end_of_the_method_named_read_where_we_do_other_unrelated_things_after_reading.

post_read should immediately follow the reader.read call because it gives hook-access to the whole collection. It is here, for example, that userland could re-arrange the collection for "sticky" posts before the rather heavy-handed call to truncate the collection which, arguably, does not even belong in this method.

it's post_read, not end_of_the_method_named_read_where_we_do_other_unrelated_things_after_reading.

post_read should immediately follow the reader.read call because it gives hook-access to the whole collection. It is here, for example, that userland could re-arrange the collection for "sticky" posts before the rather heavy-handed call to truncate the collection which, arguably, does not even belong in this method.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 9, 2015

Member

@stevecrozz Is this ready to go?

Member

parkr commented May 9, 2015

@stevecrozz Is this ready to go?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 10, 2015

Member

Merging. Feel free to add further feedback as we approach the final release!

Member

parkr commented May 10, 2015

Merging. Feel free to add further feedback as we approach the final release!

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

@parkr parkr merged commit ce9fcfa into jekyll:master May 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 May 10, 2015

@imathis

This comment has been minimized.

Show comment
Hide comment
@imathis

imathis May 10, 2015

Contributor

I just pushed an octopress-multilingual pull-request for merging Jekyll hooks support. It worked really well and was nice to integrate. Thanks for your work on this @stevecrozz and everyone else who helped! 👍

Contributor

imathis commented May 10, 2015

I just pushed an octopress-multilingual pull-request for merging Jekyll hooks support. It worked really well and was nice to integrate. Thanks for your work on this @stevecrozz and everyone else who helped! 👍

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 10, 2015

Member

Hooray!!! 🎉

Member

parkr commented May 10, 2015

Hooray!!! 🎉

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz May 11, 2015

Contributor

Very cool. Thanks for all the help, everyone. I know there were a bunch of other ideas up there that didn't make it into this first round but it doesn't have to end here. I'll look forward to seeing and reviewing more hook-related work.

Contributor

stevecrozz commented May 11, 2015

Very cool. Thanks for all the help, everyone. I know there were a bunch of other ideas up there that didn't make it into this first round but it doesn't have to end here. I'll look forward to seeing and reviewing more hook-related work.

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