Closed dgw closed 10 months ago
That'll teach me to skip at least make lint
because "it's only a docs patch". 😅
this would be better with at least one automated test, probably in
test/test_coretasks.py
, by following the same ideas as thetest_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 Identifier
s 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 Identifier
s it's created and then all of them could be updated if the CASEMAPPING
changes. But that kinda sounds like overkill…
I pushed new stuff based on IRC discussion, not just out of the blue. I promise!
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.
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 :)
I already approved that years ago. :sunglasses:
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.
Description
Here is a shortcut way of getting a new
SopelIdentifierMemory
with theidentifier_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
andchannels
automatically adds this to test coverage:(NB: That is an older coverage run, from before I expanded the docstring.)
Checklist
make qa
(runsmake lint
andmake test
)