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

Make overriding cnf-handler work #2331

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@faho
Member

faho commented Aug 24, 2015

Do this by renaming the __fish_command_not_found_handler used during
startup to __fish_startup_command_not_found_handler. That allows us to
check if __fish_command_not_found_handler has been defined and skip the
setup of the normal one.

Now disabling cnf-handling can be done via defining an empty
__fish_command_not_found_handler in config.fish.

It's quite possible that I haven't understood the full complexity of the situation so I've decided to open a PR instead of just pushing. Of course solving #845 could make all of this moot.

Make overriding cnf-handler work
Do this by renaming the __fish_command_not_found_handler used during
startup to __fish_startup_command_not_found_handler. That allows us to
check if __fish_command_not_found_handler has been defined and skip the
setup of the normal one.

Now disabling cnf-handling can be done via defining an empty
__fish_command_not_found_handler in config.fish
@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Aug 27, 2015

Member

Looks good to me.

Member

zanchey commented Aug 27, 2015

Looks good to me.

Erase startup cnf-handler early
Simplifies the code a bit - in particular it removes the special-casing
from the startup handler.
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 30, 2015

Member

I re-simplified it a bit (not sure why I didn't do it that way right away).

I intend to merge this tomorrow unless anyone objects.

Member

faho commented Aug 30, 2015

I re-simplified it a bit (not sure why I didn't do it that way right away).

I intend to merge this tomorrow unless anyone objects.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 31, 2015

Member

There's a point here that's probably not as intuitive as it should be - if you define __fish_command_not_found_handler without "--on-event fish_command_not_found", it will disable the cnf-logic without being executed itself. (I also don't quite like users fiddling with the dunderscore "namespace", but I guess renaming it would constitute an API break which I also don't like)

There's no great solution I can see for it - one would be to have a way to check if an event handler is registered for a particular event, a way to register already defined functions as event handlers might work as well (if it can't get double-defined). The former is kinda nice, the latter is kinda un-orthogonal (since this specially named function would magically become an event handler no matter how it's defined).

Regardless, this solves a problem and doesn't hurt or prevent doing something better, so I'll merge this anyway now.

Member

faho commented Aug 31, 2015

There's a point here that's probably not as intuitive as it should be - if you define __fish_command_not_found_handler without "--on-event fish_command_not_found", it will disable the cnf-logic without being executed itself. (I also don't quite like users fiddling with the dunderscore "namespace", but I guess renaming it would constitute an API break which I also don't like)

There's no great solution I can see for it - one would be to have a way to check if an event handler is registered for a particular event, a way to register already defined functions as event handlers might work as well (if it can't get double-defined). The former is kinda nice, the latter is kinda un-orthogonal (since this specially named function would magically become an event handler no matter how it's defined).

Regardless, this solves a problem and doesn't hurt or prevent doing something better, so I'll merge this anyway now.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 31, 2015

Member

Squashed merge as 2f3123e.

Member

faho commented Aug 31, 2015

Squashed merge as 2f3123e.

@faho faho closed this Aug 31, 2015

@faho faho deleted the faho:cnf branch Aug 31, 2015

@faho faho added this to the next-2.x milestone Sep 26, 2015

@faho faho added the enhancement label Sep 26, 2015

@zanchey zanchey referenced this pull request Sep 29, 2015

Closed

Custom event handlers #2231

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