Subscriber method registration bug about tukio HOT 6 CLOSED

And done. All flows through the OrderedListenerProvider should throw consistent errors, and only catch the desired functions when parsing.

In the absence of other cleanup concerns, I'm closing this out.

from tukio.

I've added a PR here.
It changes the string comparison to catch only methods starting with 'on', not simply containing it.

It intentionally makes no changes to the exacerbating condition of fatal errors on missing typehints, as that is out of my scope of experience with the project (and might be intentional.)

from tukio.

The over-eager "on" parsing is fixed by #8, now merged.

The failure of methods that have no type hint is expected; a method with no type hint cannot be auto-registered because the library has no idea what to register it for. That said, I agree that a more useful error message would be an improvement. Probably the best would be to modify OrderedListenerProvider to check for a missing type, and throw a more specific and helpful exception in that case. (That way it works for trying to register a type-less listener from any place.)

@Niflthaena Do you want to take a crack at it?

from tukio.

I certainly could. It raises some concerns about changing behavior (replacing a Fatal error with an Exception that could accidentally be caught by existing code), but it's more flexible for new development.

I was mostly concerned because the over-eager parsing was leading to uncatchable errors outside my listeners, making it much harder to integrate into existing code. Either issue alone is much less of a problem; bad listeners that are never called is just a performance hit, and writing a listener wrong and getting a development-time error is good regardless of if it's a Fatal or an Exception.

Regardless, if you think the slight risk of breaking changes is acceptable, I'll put in a PR later.

from tukio.

Changing an unclear fatal error into a self-documenting fatal error (what an uncaught exception is) doesn't qualify as a breaking change in my book, so go for it. It should probably have a dedicated exception class.

from tukio.

PR opened. It syncs up some cases that were argument exceptions and some that were fatal errors to make things more readable and consistent. Detailed notes in the pull request.

from tukio.