isupport: parse CLIENTTAGDENY to a custom type that understands *

[dgw]

This replaces directly passing through the CLIENTTAGDENY value as a plain string.

Consumers of the CLIENTTAGDENY token can either check whether a tag name is directly contained in the set, or go through the is_denied() method to apply wildcard logic if appropriate.

The ClientTagDeny object's membership tests are case insensitive.

Resolves #2501 in, possibly, "overkill" fashion. I like offering plugin authors some convenience, but I'll understand if it would be preferred to just use a simple list or set.

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 lint and make test)
  • There were, but this time I actually remembered to lint and test locally before opening a PR 😅
• I have tested the functionality of the things this change touches

[Exirel]

I get the idea. I'm trying to think about the use case:

• only the server can set/add/update this value, so it could be a read-only object
• a client needs to know what is denied... so the is_denied method is important
• maybe it wants to know if clientdeny.all(*tags) or clientdeny.any(*tags) are denied too?
• but does it really need to know the exact list of what's inside? and if so... maybe just make the list a public frozenset?

I don't think you really need to implement all the magic methods, I'd replace them with this instead:

class ClientTagDenied:
    def __init__(self, *tags: str) -> None:
        self.tags = frozenset(...)
    def is_denied(self, tag: str) -> bool:
        """Tell if tag is denied."""
        ...
    def all(self, *tags: str) -> bool:
        """Tell if all tags are denied, not just some of them."""
        ...

    def any(self, *tags: str) -> bool:
        """Tell if at least one tag is denied."""
        ...

I think you could keep more or less your unit tests - except for the case-insensitive comparison. I think it's an OK tradeoff, given the niche usage for that information.

What do you think?

[dgw]

I am trying to keep the interface simple, using the in operator where possible. In truth, I don't think my original implementation actually needed an is_denied() method… just a smarter __contains__(). Oops.

Also oops: Tag names MUST be treated as "case-sensitive opaque identifiers", so the case-insensitive comparison I added for convenience is spec-breaking.

Making the object read-only makes sense to me. It works as long as a plugin doesn't grab and hold a long-lived reference to it (since the server can re-advertise the CLIENTTAGDENY token if its configuration changes).

I don't have the energy to do it right now, but this needs a rewrite with a closer eye to the spec. (Another example: * MUST be the first item in the list, so there's no reason to use a condition like '*' in self._data.) For now, back to draft it goes.

[Exirel]

I am trying to keep the interface simple, using the in operator where possible. In truth, I don't think my original implementation actually needed an is_denied() method… just a smarter __contains__(). Oops.

The more I think about the issue, the more I think the bot itself should be the one dealing with allowed/denied tags, and offer a convenient interface, and less ISUPPORT itself.

But for that, I need to pursue my exploration of implementing client-only tag in Sopel... and I've barely touched the surface so far.

It works as long as a plugin doesn't grab and hold a long-lived reference to it

Exactly like anything that is in ISUPPORT: a plugin shall not make a copy and use that as a reference, since it can change at anytime.

[dgw]

The more I think about the issue, the more I think the bot itself should be the one dealing with allowed/denied tags, and offer a convenient interface, and less ISUPPORT itself.

Counter: The bot shouldn't have an infinitely expanding API surface. I will entertain the idea that bot.isupport mightn't be the best place for plugins to check tag moderation rules, but let's also please consider that bot.another_new_method() is not necessarily the best approach either. Things that are related to bot.isupport data and nothing else should ideally stay contained within that property.

This could be (almost-)3am-brain talking… Yet sometimes I think it's odd that we've added things like bot.make_identifier() or bot.safe_text_length() there. (No, I will not try to brainstorm any more on this. It's late and time for 🛌! 😅)