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

Add hooks system for packages #286

Merged
merged 1 commit into from
Jul 18, 2016
Merged

Add hooks system for packages #286

merged 1 commit into from
Jul 18, 2016

Conversation

sagebind
Copy link
Member

This pull request adds a new (or rather, revamped) "hooks" system for packages to tap into during the package's lifetime. Similar to Git hooks, OMF hooks are Fish shell scripts located in a hooks/ directory inside a package. When a particular hook is triggered, the corresponding script is run, if it exists.

The following hooks are implemented:

  • install; Triggered when the package is first installed via omf install. Useful for installing custom dependencies.
  • update: Triggered when the package is being updated via omf update. Useful for updating submodules, etc.
  • uninstall: Triggered when the package is removed via omf remove. Subscribers can use the event to clean up custom resources, etc.

For consistency, the uninstall hook is now in the hooks/ directory, same as the other hooks.

This PR has a weird relationship with #260; it may or may not need to be merged after #260 is. It is based off of master though.

Only the English docs are updated; the docs team can update translations separately if this is merged.

@derekstavis
Copy link
Member

I always wanted install and update hooks! This hooks are very useful for Git submodule usage or other custom code that runs only once after install/update. I see lots of use cases for this!

I had a fast review of the PR, and I raised some concerns about the concepts:

  • Do we want to move hooks to a entire new directory instead of package root directory, as we use today? I find it appropriate, but I'm still thoughtful about foreign packages (not under Oh My Fish org).
  • Calling hooks in a subshell loads entire Oh My Fish again in the subprocess. I don't know whether this can cause issues on update or uninstall hooks, or even if it makes sense to be done this way.
  • I see that the update hook is called twice. In this case it would be more appropriate to use separate before_update and after_update hooks, rather than calling the same hook twice, if this was your intention.

Aside from this concerns I love your initiative! Thanks for such PR 🎉

@sagebind
Copy link
Member Author

Thanks for the feedback. Let me address each of your points:

  1. Personal preference, but I don't like a messy repo root directory 😉. My concern actually was to benefit (or at least not break) foreign packages. A lot of them still put functions to be loaded on init in the package root. And if they had a function named the same thing as a hook... it would be run as a hook instead of being a function. Only issue then is that <pkg>/uninstall.fish still needs to be addressed for BC sake.

  2. I was playing around with my implementation and put

    cd $argv[1]
    git submodule update

    in a plugin hook as a test. But this changed my current directory. Of course, right? But I didn't think about that. Maybe there's a better way, but to prevent "fiddling with things", I used a subshell. Not really ideal.

  3. That's a Git mistake on my part. 😱 It should be triggered only once.

@oranja
Copy link
Contributor

oranja commented Mar 26, 2016

I was going to reinforce @derekstavis concerns regarding the double update and the subshell invocation, but you got there first.
Having the hooks in a sub-directory seems like a better idea to me too. It should help to preserve compatibility with other frameworks.

What concerns me the most right now is that I'm still not convinced about use cases.
You both seem to have git submodules in mind, but if submodules are the main use of these hooks and it becomes something we wish to encourage and support, why not just target submodules directly? Like we intend to do with requirements and key-bindings?
Does anyone have an argument for why each package should manage its own submodules (if it becomes something of a habit), or has any other practical use case for these hooks to present?

@derekstavis
Copy link
Member

I don't believe submodules should be a common thing to deserve a implementation inside the framework.

For example, nvm and rvm plugins should depend on user-installed dependencies, instead of bundling them using submodules.

@oranja
Copy link
Contributor

oranja commented Mar 26, 2016

Then the question remains: which type of packages do need submodules? or the other hooks for any purpose?
Until I'm convinced I remain neutral on this. On the one hand change is good. On the other hand growing the code base is not always the best.

@sagebind
Copy link
Member Author

Mostly packages that have third-party shell scripts, like spark or fasd. Using submodules has the advantage of reducing the number of install / update steps, since otherwise you need to download a shell script by hand and put it in the path somewhere. And since it's just shell scripts, compilation, compilers, interpreters, etc. are not a concern.

Now things that need binaries should probably avoid submodules, since it would be better to install the binaries with the system package manager.

@sagebind
Copy link
Member Author

Just pushed a commit that improves the following:

  • source is used to run hook scripts.
  • Instead of $argv, the $package and $path variables are provided for hook scripts.
  • The working directory inside a hook script is the package root directory.
  • As long as the hook script isn't too naughty, the current user working directory is preserved.

@oranja
Copy link
Contributor

oranja commented Mar 26, 2016

The code looks great now IMO.

@@ -0,0 +1,20 @@
function omf.packages.hook -a path hook
Copy link
Member

Choose a reason for hiding this comment

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

Not being nitpicky, but omf.packages.hook.run sounds more descriptive regarding to the action which the function performs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough. How about omf.packages.run_hook?

Copy link
Member

Choose a reason for hiding this comment

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

Was my second option 😝

@sagebind
Copy link
Member Author

Renamed omf.packages.hook function to omf.packages.run_hook, rebased, squashed, and removed verbosity from run_hook.

@sagebind sagebind added this to the v2 milestone Apr 3, 2016
@sagebind sagebind added the status: on hold Issue on hold label May 10, 2016
@sagebind sagebind removed the status: on hold Issue on hold label Jul 14, 2016
@derekstavis
Copy link
Member

Im 👌 for merging

@sagebind sagebind merged commit e9fb8ff into oh-my-fish:master Jul 18, 2016
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.

None yet

3 participants