safety: rework

[half-duplex]

Description

This fixes (or works towards fixing) a bunch of issues with safety.py:

• .safety doesn't say what the current state is
• Uses VirusTotal API v2 (though they have no current plans to deprecate it) instead of v3
• Doesn't find URLs in messages containing anything else ("foo http://example.com" fails)
• Doesn't make local cache accessible to other plugins
• Doesn't use already-cached VT results in local mode
• Doesn't allow configuration of local or strict defaults
• url.py was not checking the domain blacklist and was also failing to handle "foo http://example.com"
• Other tweaks and improvements

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

[dgw]

Non-blocking style/correctness nits.

[half-duplex]

I meant to keep this as at least two separate commits, but accidentally squashed them together 😭
Re-requesting reviews because of nontrivial logic changes and new command.

[dgw]

OK, I have been nudged into "actual review" territory.

[dgw]

I'm not going to dig into this much more. Just noting that I added another flake8 plugin to the todo list in #1765 after seeing the mess of imports that type-hinting this plugin added.

[dgw]

I actually thought this was ready to go, but apparently not. Now that I've gone through it once more it's time to find out why GH still shows it in my "Approved 8.0.0" view, and make it not do that.

Stale/re-requested

[dgw]

All good from my perspective. Probably still wait for re-review from @Exirel to be sure everything he requested is satisfactory too.

[Exirel]

While we are upgrading the safety plugin, I think we should replace all the "safety_cache" hard-coded key into a constant, so bot.memory["satefy_cache"] would become bot.memory[SAFETY_CACHE_KEY].

They are also some probable nitpick around docstrings that lack a period here and there, but I couldn't be bothered to check them all.

[half-duplex]

Should url.py import safety.SAFETY_CACHE_KEY, or just use the string?
Also considering whether I should just find a way to merge safety_cache and safety_cache_local...

[Exirel]

Should url.py import safety.SAFETY_CACHE_KEY, or just use the string?

You can import it if you want, both option are fine by me. We can accept that URL totally depends on the safety plugin, because it kind of does. As long as safety is part of core, anyway. Which shouldn't change yet.

[half-duplex]

I don't think URL does depend on the safety plugin - if safety is absent, url just skips those checks. Importing the constant would require a conditional to keep the dependency separate.

[Exirel]

I don't think URL does depend on the safety plugin

It does, through the data structure that safety exposes: safety_cache[url]["positives"] > 0. This small line implies that safety_cache[url] (when it exists) is a dict that always contains a positive key that is always a numeric that you can compare to 0.

[half-duplex]

Ok, if something uses safety.py's key without following the format there will be issues, but url.py doesn't fail to perform any of its functions if safety.py is absent. I can add two more .get()s if you want, but either way I think I prefer using the string.

[dgw]

If the code only even tries to read the safety info if the safety plugin is available, you minimize the chance of stepping on something else that has coopted its key name. That, to me, would be the main advantage of e.g.

try:
    from sopel.modules.safety import SAFETY_CACHE_KEY
except ImportError:
    SAFETY_CACHE_KEY = None

# ... other code ...

# in the relevant function
    if SAFETY_CACHE_KEY is not None:
        # use it

Is that perfect? No, because safety might not be enabled. But there's also bot.has_plugin('safety') for additional sanity checking if you want that.

[Exirel]

both option are fine by me

I'm still OK with that, as I don't think it matters too much at the moment. The Safety feature of Sopel would probably be better with a proper interface dedicated to that, instead of a Plugin. So for now, a string is fine by me. I'm just glad we could talk a bit about it, even tho it's nothing more than nitpicking at this point.