New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run hooks in priority order. #5157

Merged
merged 1 commit into from Sep 28, 2016

Conversation

Projects
None yet
4 participants
@stevecheckoway
Contributor

stevecheckoway commented Jul 28, 2016

Low priority hooks are being run before higher priority hooks. This is easy to
demonstrate with the following plugin:

1.upto(10).each do |n|
  Jekyll::Hooks.register :site, :after_reset, priority: Jekyll::Hooks::PRIORITY_MAP[:low] do
    puts "Low #{n}"
  end
  Jekyll::Hooks.register :site, :after_reset, priority: Jekyll::Hooks::PRIORITY_MAP[:normal] do
    puts "Normal #{n}"
  end
  Jekyll::Hooks.register :site, :after_reset, priority: Jekyll::Hooks::PRIORITY_MAP[:high] do
    puts "High #{n}"
  end
end

Sorting by the negative of the priority and then by the order the hook was
added does the right thing.

@stevecheckoway

This comment has been minimized.

Show comment
Hide comment
@stevecheckoway

stevecheckoway Jul 28, 2016

Contributor

Hmm. Confusingly, the feature file indicates the low priority hooks should run first. That's counter-intuitive and disagrees with plugin priority meanings.

Either the feature file should be fixed, or the - in the patch removed so that the hook priority is [priority, @hook_priority.size].

Either way, comparing as a floating point number gives seriously broken results. From the plugin in the commit message, without the patch, I get

Low 1
Low 5
Low 6
Low 7
Low 8
Low 9
Low 10
Low 2
Low 3
Low 4
Normal 1
Normal 4
Normal 5
Normal 6
Normal 7
Normal 8
Normal 9
Normal 10
Normal 2
Normal 3
High 4
High 5
High 6
High 7
High 1
High 8
High 9
High 10
High 2
High 3
Contributor

stevecheckoway commented Jul 28, 2016

Hmm. Confusingly, the feature file indicates the low priority hooks should run first. That's counter-intuitive and disagrees with plugin priority meanings.

Either the feature file should be fixed, or the - in the patch removed so that the hook priority is [priority, @hook_priority.size].

Either way, comparing as a floating point number gives seriously broken results. From the plugin in the commit message, without the patch, I get

Low 1
Low 5
Low 6
Low 7
Low 8
Low 9
Low 10
Low 2
Low 3
Low 4
Normal 1
Normal 4
Normal 5
Normal 6
Normal 7
Normal 8
Normal 9
Normal 10
Normal 2
Normal 3
High 4
High 5
High 6
High 7
High 1
High 8
High 9
High 10
High 2
High 3

@parkr parkr modified the milestone: 3.3 Jul 28, 2016

@stevecheckoway stevecheckoway referenced this pull request Aug 1, 2016

Closed

Hooks: documentation and consistency #5172

4 of 10 tasks complete
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 6, 2016

Member

Agreed - high priority plugins should be run first, if only to be consistent with the other plugin infrastructure which does high to low.

Member

parkr commented Aug 6, 2016

Agreed - high priority plugins should be run first, if only to be consistent with the other plugin infrastructure which does high to low.

Run hooks in priority order.
Low priority hooks are being run before higher priority hooks. This is easy to
demonstrate with the following plugin:

    1.upto(10).each do |n|
      Jekyll::Hooks.register :site, :after_reset, priority: Jekyll::Hooks::PRIORITY_MAP[:low] do
        puts "Low #{n}"
      end
      Jekyll::Hooks.register :site, :after_reset, priority: Jekyll::Hooks::PRIORITY_MAP[:normal] do
        puts "Normal #{n}"
      end
      Jekyll::Hooks.register :site, :after_reset, priority: Jekyll::Hooks::PRIORITY_MAP[:high] do
        puts "High #{n}"
      end
    end

Sorting by the negative of the priority and then by the order the hook was
added does the right thing.
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 7, 2016

Contributor

plugin priority makes little sense for hooks and plugins, I don't even know why that is there. Everybody can just delegate themselves as high or low, and most don't even matter. For hooks this is even more so, hooks should have a name so that you can insert before or after a specific hook if you need to, but priorities make little sense because everybody will just be high or low.

Ultimately priorities that are not weighted by the user, have little meaning without named meaning.

Contributor

envygeeks commented Aug 7, 2016

plugin priority makes little sense for hooks and plugins, I don't even know why that is there. Everybody can just delegate themselves as high or low, and most don't even matter. For hooks this is even more so, hooks should have a name so that you can insert before or after a specific hook if you need to, but priorities make little sense because everybody will just be high or low.

Ultimately priorities that are not weighted by the user, have little meaning without named meaning.

@stevecheckoway

This comment has been minimized.

Show comment
Hide comment
@stevecheckoway

stevecheckoway Aug 7, 2016

Contributor

Can't users get the same behavior of named hooks simply by using distinct priorities? They're not constrained to :low, :normal, or :high.

Contributor

stevecheckoway commented Aug 7, 2016

Can't users get the same behavior of named hooks simply by using distinct priorities? They're not constrained to :low, :normal, or :high.

@parkr

parkr approved these changes Sep 28, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 28, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Sep 28, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 6847b60 into jekyll:master Sep 28, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Awaiting approval from at least 2 maintainers.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Sep 28, 2016

@stevecheckoway stevecheckoway deleted the stevecheckoway:fix-hooks-priority-order branch Sep 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment