#3870 trigger hooks by owner symbol #3871

Merged
merged 1 commit into from Aug 23, 2015

Conversation

Projects
None yet
4 participants
@stevecrozz
Contributor

stevecrozz commented Jul 27, 2015

Addresses #3870 by changing the trigger interface to work with symbol owners rather than object owners, making the hooks system more flexible.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 27, 2015

Contributor

Does this require a documentation update?

/cc @parkr @imathis

Contributor

envygeeks commented Jul 27, 2015

Does this require a documentation update?

/cc @parkr @imathis

@stevecrozz

This comment has been minimized.

Show comment
Hide comment
@stevecrozz

stevecrozz Jul 27, 2015

Contributor

The old docs still apply since the only real change was for the 'trigger' method which is undocumented.

Contributor

stevecrozz commented Jul 27, 2015

The old docs still apply since the only real change was for the 'trigger' method which is undocumented.

lib/jekyll/convertible.rb
@@ -144,6 +144,17 @@ def type
end
end
+ # returns the owner symbol for hook triggering
+ def hook_owner
+ if is_a?(Draft)

This comment has been minimized.

@parkr

parkr Jul 27, 2015

Member

This is superfluous, I believe. Draft.is_a? Post should be true.

@parkr

parkr Jul 27, 2015

Member

This is superfluous, I believe. Draft.is_a? Post should be true.

This comment has been minimized.

@stevecrozz

stevecrozz Jul 27, 2015

Contributor

You are right. I'll make that change.

@stevecrozz

stevecrozz Jul 27, 2015

Contributor

You are right. I'll make that change.

- def self.trigger(instance, event, *args)
- owner_symbol = OWNER_MAP[instance.class]
-
+ def self.trigger(owner, event, *args)

This comment has been minimized.

@parkr

parkr Aug 3, 2015

Member

i think we can keep using instance, and just accept either a symbol or an instance of something. instead of the callers having to deal with this complexity, this class could handle it.

@parkr

parkr Aug 3, 2015

Member

i think we can keep using instance, and just accept either a symbol or an instance of something. instead of the callers having to deal with this complexity, this class could handle it.

This comment has been minimized.

@stevecrozz

stevecrozz Aug 12, 2015

Contributor

The situation that prompted this change was one where a user had subclassed Page. Let's say you do that by declaring class MyPage < Jekyll::Page; end. The expected behavior is at certain stages of the build, Jekyll should trigger hooks.

The problem is Jekyll doesn't currently know that MyPage should trigger :page hooks. MyPage relies on its superclass and potentially other internal Jekyll components to trigger hooks at the right times. But if what we have is an instance of MyPage, how can we translate that into the right hash key, :page in this case, to look up the hooks.

I had a few ideas and this PR represents the simplest, but maybe also the ugliest from the caller's perspective. But here's a list of the ideas I've had so far. Let me know if you have any others:

  1. Require the trigger caller to pass they key in the trigger method call (this PR)

  2. Give every type of owner an instance method, class method or constant (something heritable) that calls out the owner symbol. Take this change to Jekyll::Page for example:

module Jekyll
  class Page
    def self.hook_owner; :page; end
  end
end

Instances of Jekyll::Page and instances of Jekyll::Page descendants could now just ask the instance of MyPage or instance of Jekyll::Page for its .class.hook_owner and know how to find the hooks. This could essentially replace the lookup table at the top of Jekyll::Hooks and make each hook owner declare such a method.

The idea is the same, but there could be some more sugary syntax:

module Jekyll
  class Page
    ...
  end
  Hooks.add_owner :page, Page
end
  1. Make the Hooks class search the class hierarchy of the instances passed in. For example if you pass in an instance of MyPage (subclass of page), Hooks would quiz that instance to see if it is_a?(Jekyll::Page) and if so trigger :page hooks. If not, ask to see if it is_a?(Jekyll::Post) and so on until it has checked all the possible hook owners it could be.
@stevecrozz

stevecrozz Aug 12, 2015

Contributor

The situation that prompted this change was one where a user had subclassed Page. Let's say you do that by declaring class MyPage < Jekyll::Page; end. The expected behavior is at certain stages of the build, Jekyll should trigger hooks.

The problem is Jekyll doesn't currently know that MyPage should trigger :page hooks. MyPage relies on its superclass and potentially other internal Jekyll components to trigger hooks at the right times. But if what we have is an instance of MyPage, how can we translate that into the right hash key, :page in this case, to look up the hooks.

I had a few ideas and this PR represents the simplest, but maybe also the ugliest from the caller's perspective. But here's a list of the ideas I've had so far. Let me know if you have any others:

  1. Require the trigger caller to pass they key in the trigger method call (this PR)

  2. Give every type of owner an instance method, class method or constant (something heritable) that calls out the owner symbol. Take this change to Jekyll::Page for example:

module Jekyll
  class Page
    def self.hook_owner; :page; end
  end
end

Instances of Jekyll::Page and instances of Jekyll::Page descendants could now just ask the instance of MyPage or instance of Jekyll::Page for its .class.hook_owner and know how to find the hooks. This could essentially replace the lookup table at the top of Jekyll::Hooks and make each hook owner declare such a method.

The idea is the same, but there could be some more sugary syntax:

module Jekyll
  class Page
    ...
  end
  Hooks.add_owner :page, Page
end
  1. Make the Hooks class search the class hierarchy of the instances passed in. For example if you pass in an instance of MyPage (subclass of page), Hooks would quiz that instance to see if it is_a?(Jekyll::Page) and if so trigger :page hooks. If not, ask to see if it is_a?(Jekyll::Post) and so on until it has checked all the possible hook owners it could be.

This comment has been minimized.

@envygeeks

envygeeks Aug 12, 2015

Contributor

The cleanest and most direct IMO is option one. It's not just that area we had trouble, it was within Jekyll itself too. The problem within Jekyll's own source is what actually started this entire conversation weeks before this one spawned the change.

Where at one point we were trying to add in hooks for Jekyll's internal configurations so we could hook out dozens of points, we couldn't get a subclass because it was Hash because the code did not speak for itself. That's too broad, so we needed to make things open and explicit in that a user should have to pass an owner and a hook, this makes it so that anybody can call any hook at any point for any reason.

It's also not just us we need to worry about, it's plugins too, the hook system is only as valuable as it's usage so we have really two problems here:

  1. How do we trigger hooks.
  2. How do we let plugins have their own hooks so this: https://github.com/jekyll-assets/jekyll-assets/blob/master/lib/jekyll/assets/hook.rb doesn't have to happen.

IMO there should be no basis for resistance against changing the API to the more explicit version because hooks haven't been around long enough to gain usage, there was never any option for plugins to use them directly (which means a plugin can't be broken) and the messaging to hooks was only used internally so this shouldn't in theory and in testing break anything like Octopress, because it's only asking to add a hook to a point... we are the ones who are messaging the hooks on the point... not Octopress.

@envygeeks

envygeeks Aug 12, 2015

Contributor

The cleanest and most direct IMO is option one. It's not just that area we had trouble, it was within Jekyll itself too. The problem within Jekyll's own source is what actually started this entire conversation weeks before this one spawned the change.

Where at one point we were trying to add in hooks for Jekyll's internal configurations so we could hook out dozens of points, we couldn't get a subclass because it was Hash because the code did not speak for itself. That's too broad, so we needed to make things open and explicit in that a user should have to pass an owner and a hook, this makes it so that anybody can call any hook at any point for any reason.

It's also not just us we need to worry about, it's plugins too, the hook system is only as valuable as it's usage so we have really two problems here:

  1. How do we trigger hooks.
  2. How do we let plugins have their own hooks so this: https://github.com/jekyll-assets/jekyll-assets/blob/master/lib/jekyll/assets/hook.rb doesn't have to happen.

IMO there should be no basis for resistance against changing the API to the more explicit version because hooks haven't been around long enough to gain usage, there was never any option for plugins to use them directly (which means a plugin can't be broken) and the messaging to hooks was only used internally so this shouldn't in theory and in testing break anything like Octopress, because it's only asking to add a hook to a point... we are the ones who are messaging the hooks on the point... not Octopress.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 17, 2015

Member

Ok, I'm fine moving forward with this PR. Anything else we need?

Member

parkr commented Aug 17, 2015

Ok, I'm fine moving forward with this PR. Anything else we need?

parkr added a commit that referenced this pull request Aug 23, 2015

@parkr parkr merged commit 8927898 into jekyll:master Aug 23, 2015

1 check passed

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

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