plugins: handle URL callbacks in new rule system

[Exirel]

Description

TL;DR:

• URL callbacks are now part of the new rule system
• everything related to bot.memory['url_callbacks'] is deprecated (but not marked as such in this PR)
• the match parameter is obsolete and shouldn't be used anymore - it still work, but not needed anymore
• there is a new plugin.url_lazy(loader) that takes a loader and it builds regexes when it needs to
• yeah, I did rewrote the entire world, again

The idea is to have url and url_lazy to be part of the rule system: the bot ask its rule manager to register them, based on what they have. As a result, URL callback are like every other rules: they can take advantage of all the decorators in sopel.plugin and its feature such as output prefix and rate limiting.

In effect, that makes the bot.memory['url_callbacks'] deprecated. This PR takes care of being backward compatible, and we'll have to decide when we want to remove it. (I think Sopel 9 is a good target?)

However, I had to take a difficult decision for the trigger and the match parameters: now, the trigger object is based on the match object, making the match parameter obsolete. (as they contain the same information) I don't think that's a problem, as the trigger object still contains the full IRC line, and it couldn't be used reliably anyway because it would know about the first URL in the IRC message anyway.

Note that doesn't break callable that are supposed to be both a rule/command and a URL callback, because it's not possible: the loader doesn't allow it, and the rule/command status of the callable takes precedence.

Checklist

• I have read CONTRIBUTING.md
• I can and do license this contribution under the EFLv2
• No issues are reported by make qa (runs make quality and make test)
• I have tested the functionality of the things this change touches

Related issues/PRs

• Replaces bot: Support multiple handlers for a URL pattern #1805
• Closes Update URL callback registration mechanism #1804 (in conjunction with other changes already made to URL handling)
• Closes @url callables ignore some other plugin decorators #1816
• Requires coretasks: replace module.label by plugin.label #1906 first (should be rebased after its merged)

[dgw]

Well this is exciting.

• everything related to bot.memory is deprecated

I hope this means only the URL-related bits 😂

• the match parameter is obsolete and shouldn't be used anymore

I'm not sure about this. Maybe I'll understand why when I actually have time to review the patch, but at first glance it sounds arbitrary. AFAIK it exists to make handing both URLs and commands/rules with the same callable.

[lgtm-com]

This pull request introduces 1 alert when merging cea2bfc into a5a6e2d - view on LGTM.com

new alerts:

• 1 for Unused import

[Exirel]

I hope this means only the URL-related bits joy

Yes! Only for the URL callback related keys in bot.memory.

AFAIK it exists to make handing both URLs and commands/rules with the same callable.

I'd be on board with that, but the loader.clean_module doesn't agree with us:

sopel/sopel/loader.py

Lines 205 to 215 in 65fb76a

	if getattr(obj, '__name__', None) == 'shutdown':
	shutdowns.append(obj)
	elif is_triggerable(obj):
	clean_callable(obj, config)
	callables.append(obj)
	elif hasattr(obj, 'interval'):
	clean_callable(obj, config)
	jobs.append(obj)
	elif hasattr(obj, 'url_regex'):
	clean_callable(obj, config)
	urls.append(obj)

(taken from master as of today). What this if/elif section does is that, if the callable is a generic rule or a command, it won't even try to register it as an URL Callback. So the current code doesn't allow it.

That something that I can probably change, and I'd suggest that in another PR. This one is already quite big.

And I reassert that I'm so glad that you took the time to raise this, because I didn't think about it, and it's a great opportunity to improve even further. Thank you. ❤️

[lgtm-com]

This pull request introduces 1 alert when merging 39a378d into a5a6e2d - view on LGTM.com

new alerts:

• 1 for Unused import

[dgw]

You rebased this before merge, @Exirel? It's showing commits from #1898 too.

[dgw]

I like where this is headed. Blanket comments:

• Use of capital C in "URL Callback" vs. "URL callback" is inconsistent both across this patch and across the existing code/docs. Could be worth a global, case-sensitive find-and-replace if you get bored late one night and don't want to do any "real" coding. 😁
• "an URL" vs. "a URL" (we spoke about this one on IRC a bit)

[Exirel]

@dgw I'll have a look at the Travis error on the .ip test tomorrow. Meanwhile, you can have a look, I fixed the PR. I think it's good now.

[dgw]

@Exirel The .ip "failure" is because the GeoIP database updated to add fields for the test IP that the current regex doesn't expect. Nothing to do with this PR. I thought fetching the DB was mocked out by the VCR patches I did, but apparently it still goes to get the live copy after all. I'll push a commit to fix the regex before bed, so you can keep hacking tomorrow.

[dgw]

Welp, turns out right now is right before bed. I'm not sure what to do now, since 19:19 is too early to sleep… 😝

[Exirel]

It's ready for the review! I'll edit the description.

@dgw I noted a bug that I should fix directly for the default branch, and not in this PR. But I've added it here so I can actually test live with my working dir.

[Exirel]

See also #1906 for the hotfix.

[dgw]

Commenting before review, lest I forget: Don't we space-separate names in the commit message? You have e.g. bot,rules,loader: but I'd expect bot, rules, loader:.

Hopefully I'll finish a review of this tonight or tomorrow.

[dgw]

All of this (which is mostly more docstring stuff), plus the nitpick about commit message formatting I commented earlier.

[Exirel]

@dgw some new stuffs in here! I request a new review! ❤️

[lgtm-com]

This pull request introduces 1 alert when merging d65435b into a33caf1 - view on LGTM.com

new alerts:

• 1 for Module is imported with 'import' and 'import from'

[Exirel]

* 1 for Module is imported with 'import' and 'import from'

Oh yes, that's because I have to import manually the right function depending on the Python's version: in 2.7, the function to inspect a function's parameters is not the same as the one in 3.3+. So this alarm can be ignored, and will be removed automatically when we drop support for Python 2.7.

[dgw]

❤️ making match optional for URL callables, best change ever

[Exirel]

@dgw ready to squash whenever you want.

[lgtm-com]

This pull request introduces 1 alert when merging 3c35180 into ee3fcb2 - view on LGTM.com

new alerts:

• 1 for Module is imported with 'import' and 'import from'

[dgw]

@Exirel I looked at "changes since [my] last review" and I'm happy. Squash away, at least locally, so you can move on to that documentation thing you wanted to tackle (tomorrow? IIRC)

[lgtm-com]

This pull request introduces 1 alert when merging db5bf69 into ee3fcb2 - view on LGTM.com

new alerts:

• 1 for Module is imported with 'import' and 'import from'

[Exirel]

OK on this one, I think we are good. I could potentially work on deprecating the memory/url_callback thing, but that can be left to another PR.

[dgw]

GitHub still says I've viewed all the changes, so I'll take that as confirmation that I don't need to look again now that the fixup commits are squashed. If any uncaught issues pop up, we're far enough from release that I think we'll catch them.