Hooks (Re: [Bro-Commits] [git/bro] topic/jsiwek/hook: Add new function flavor called a "hook". (e0fb9eb))

That was fast! :slight_smile:

+ hook myhook(s: string) &priority=10
+ {
+ print "priority 10 myhook handler", s;
+ s = "bye";
+ }
+
+ hook myhook(s: string)
+ {
+ print "break out of myhook handling", s;
+ break;
+ }
+
+ hook myhook(s: string) &priority=-5
+ {
+ print "not going to happen", s;
+ }

[...]

+ hook myhook("hi");

I haven't looked too closely yet but I want to mention one thing Seth
and I discussed earlier this week: we could give hooks a mandatory
boolean return value and use that instead of the "break" command:
"return T" would continue with the next hook, and "return F" stops. In
addition, the hook statement becomes an expression that returns the
boolean (i.e., T if all ran through, and F is one stopped execution).

Thinking now about it, I believe either way is fine. The boolean hook
would convey a bit more information back to the caller and allow
simple yes/no decisions that way. But I'm not sure we need that. On
the other hand, Seth didn't like the "break" so that would go away.

Wanted to put that up for discussion, not leaning strnonly either way
right now.

Robin

Jon actually convinced me in favor of "break" earlier today. Something about needing to return from these hooks feels wrong.

I was pushing for the change to making this an expression earlier today too and having a return value (I was actually pushing for a return record that had more than just if the execution was broken) and Jon made a good point about the partial inconsistency with the hook prototype.

If we had the simple boolean return from hook as an expression…

   global my_hook: hook(foobar: string): bool;

It's weird because that's not the prototype for each hook handler (not sure what to call those?) since each hook handler doesn't have a return value.

.Seth

I haven't looked too closely yet but I want to mention one thing Seth
and I discussed earlier this week: we could give hooks a mandatory
boolean return value and use that instead of the "break" command:
"return T" would continue with the next hook, and "return F" stops. In
addition, the hook statement becomes an expression that returns the
boolean (i.e., T if all ran through, and F is one stopped execution).

Thinking now about it, I believe either way is fine. The boolean hook
would convey a bit more information back to the caller and allow
simple yes/no decisions that way. But I'm not sure we need that. On
the other hand, Seth didn't like the "break" so that would go away.

Wanted to put that up for discussion, not leaning strnonly either way
right now.

I don't have a strong opinion, either, but I'm leaning toward allowing a boolean return value since it's the more flexible and most similar/familiar to how functions work.

But I do think if it had a return value, then it shouldn't mix in "break" to allow implicitly returning values, but require "return" like normal.

If you want, you can hold off on merge until this is decided, it's probably not just a trivial change to do (but not likely too difficult either).

    Jon

But I do think if it had a return value, then it shouldn't mix in
"break" to allow implicitly returning values, but require "return"
like normal.

Hmmm, good thought. Can we even get the best of both worlds by making
it all implicit: the hook definition doesn't define any return value,
but executing the hook still returns a boolean indicating whether one
of the hook implementations short-circuuted with a "break" or not.

And, maybe, we could use a different syntax instead of "break"; like
"stop" or "hook stop", to make it more explicit what happens (not that
I like either particularlly).

If you want, you can hold off on merge until this is decided, it's
probably not just a trivial change to do (but not likely too difficult
either).

Either way is fine for me, we can tweak further after the merge. I
would have merged it already but didn't get to it yet (and the rest of
the week doesn't look much better right now unfortunately). If
somebody depends on this for moving forward, please work from the
branch in the meantime.

Robin