irc, bot, builtins: add & use AbstractBot.make_identifier_memory() helper

[dgw]

Description

Here is a shortcut way of getting a new SopelIdentifierMemory with the identifier_factory kwarg filled in for you so it automatically uses the bot's awareness of casemapping from the IRC server's ISUPPORT tokens.

Manual casemapping selection is still possible by using the SopelIdentifierMemory constructor directly, of course, as before.

Using this new method in the bot to initialize its users and channels automatically adds this to test coverage:

(NB: That is an older coverage run, from before I expanded the docstring.)

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)
• I have tested the functionality of the things this change touches

[dgw]

That'll teach me to skip at least make lint because "it's only a docs patch". 😅

[SnoopJ]

LGTM, nice little ergonomics improvement for the core, builtins, and plugin developers.

[Exirel]

Code-wise, I'm OK. I think however that this would be better with at least one automated test, probably in test/test_coretasks.py, by following the same ideas as the test_handle_issuport_casemapping test.

[dgw]

this would be better with at least one automated test, probably in test/test_coretasks.py, by following the same ideas as the test_handle_issuport_casemapping test.

I'm unclear on what you want to test for that isn't already covered by use of the new method in other already-tested locations.

If it's about the memory's existing keys updating with the new casemapping behavior when CASEMAPPING comes in from the server, only the bot's own _nick is rebuilt in that case. Previously-constructed Identifiers anywhere else in the program retain their old casemapping (and their old chantypes), and this isn't unique to the SopelIdentifierMemory type. There's no reasonable way to bot.rebuild_nick() for all existing Identifier objects out there.

About the best I can do—without rewriting a bunch of stuff and taking this PR far away from the original scope—is to check adding "the same" nick that now is "not the same" under the new casemapping rules. Even that's problematic, though: if Test[a] is added to the memory before CASEMAPPING comes in (using the default rfc1459 behavior), and then CASEMAPPING=ascii comes in, you still won't be able to add test{a} to the memory because the old rfc1459 casemapping already matches that:

memory = bot.make_identifier_memory()
memory['Test[a]'] = False

# the bot gets `CASEMAPPING=ascii` in here (omitted for brevity)
# but the previous casemapping is still in effect for the previously-added key:
assert memory['Test{a}'] == False

memory['tEST{a}'] = True  # this overwrites the previous value; it doesn't add a new key
assert len(memory) == 2  # this fails; len(memory) is (still) 1

Plugins probably just shouldn't construct Identifer before the bot has connected (e.g. in setup()). Do we need a guard on these methods that raises an error if the bot isn't ready yet? I've just thought that we could (in the future) give make_identifier() a manager that keeps track of all Identifiers it's created and then all of them could be updated if the CASEMAPPING changes. But that kinda sounds like overkill…

[dgw]

I pushed new stuff based on IRC discussion, not just out of the blue. I promise!

[Exirel]

I'm unclear on what you want to test for that isn't already covered by use of the new method in other already-tested locations.

As discussed on IRC, the TL;DR is: this method need to be covered by direct invocation in the test suite, so if we stop using it internally, it is still tested and properly covered.

For everything else, the new commit with the added tests is the way to go.

[dgw]

As discussed on IRC, the TL;DR is: this method need to be covered by direct invocation in the test suite, so if we stop using it internally, it is still tested and properly covered.

I added the specific tests of bot.make_identifier() and bot.make_identifier_memory() in test/test_irc.py while I was here, to give both related methods a direct invocation from the test suite that isn't incidental to other test cases like test/test_coretasks.py's previously-existing test_handle_isupport_casemapping unit, in addition to the newly added test_handle_isupport_casemapping_identifiermemory test.

If you have another idea before merge (or after, idc), just holler @Exirel :)

[Exirel]

I already approved that years ago. 😎

[dgw]

We had ourselves a spirited discussion about the idea of having make_identifier_memory() take an argument like the SopelIdentifierMemory() constructor now can (after #2525), with a lot of shifting opinions as more considerations came up.

I think the three of us (@Exirel, @SnoopJ, and myself) landed on the idea that it's fine to ship this as-is and come back to add an optional argument in 8.1.0. At the same time, we can clean up the messy *args argspecs throughout tools.memories.

Assuming we don't change our consensus in the next few days, this will get merged and this comment should be the basis of a new issue for 8.1 regarding signatures both here and in the Memory types.