Skip to content
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

Pass hook name into plugin run functions #1053

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

itmecho
Copy link

@itmecho itmecho commented Sep 6, 2022

Implements #1050

This PR adds a third argument the plugin run function, allowing the user to easily run code only on install or update of a plugin.

It should be backwards compatible as existing functions that only accept 2 arguments will still work as before.

@@ -284,4 +284,12 @@ plugin_utils.post_update_hook = function(plugin, disp)
end)
end

plugin_utils.post_install_hook = function(plugin, disp)
return run_hook(plugin, disp, 'post_install')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than an ever expanding list of arguments, I think when you start to pass more and more args into a function it might be time to pass in a table instead, so you scale out the inputs more easily without worrying about naked args and positioning and nil-ness i.e.
I think this should be

  return run_hook(plugin, disp, { hook_name = 'post_install' })

Copy link
Author

@itmecho itmecho Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it needs a default value as it's an internal function but I've left it as an empty string. Not sure what the best thing to set it to would be?

@itmecho itmecho requested a review from akinsho September 7, 2022 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants