memories: test/fix interactions between `SopelIdentifierMemory` ↔ `dict`

[dgw]

Description

Tin. Will resolve #2524.

Checklist

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

Notes

Started as a partial, untested fix from my tablet to reserve the PR number. New day, new task list:

• [x] del d['KeY']
• [x] d.clear() (appears to work, but the object becomes a regular dict and loses the Identifier magic)
• [x] d.copy() Made a copy m and added another item to it, verifying that the new item did not appear in the original d. However, the new object m is a dict, not a SopelIdentifierMemory.
• [x] d.get('KeY') Returns the default passed to .get(), not key's value.
• [x] d.pop('KeY') (currently raises KeyError)
• [x] d.pop('KeY', 'default') (currently always returns 'default')
• [x] d.pop('NotInDict') Returns None but should raise KeyError.
• [x] d.setdefault('KeY', 'default') With 'key' in d, returns 'default' & adds 'KeY' to d as a regular string, not Identifier.
• [x] d.update(a_plain_dict) If d has Identifier('thing1') as a key, and a_plain_dict has 'ThinG1' (a regular string), they will not be merged correctly and d will now also have a regular string key called 'ThinG1'.
• [x] d | a_plain_dict Same problem as d.update(a_plain_dict).
• [x] d |= a_plain_dict Same problem as d.update(a_plain_dict) and d | a_plain_dict.
• [ ] d == a_plain_dict If a_plain_dict has the same key/value pairs but the key capitalization doesn't match Identifier's canonical (lowercase) internal representation, the equality check returns False. Decided to NAK this one and let them be unequal. It's an easily reversible decision, since this just preserves the existing behavior (so there's no worry about plugin authors screaming that we keep changing the rules 😁).
• [x] result = SopelIdentifierMemory({'KeY': 'foo'}) result is of type SopelIdentifierMemory, but without the magic. Existing keys are not converted to Identifier, and 'key' in result doesn't work as expected for that type. In fact, even 'KeY' in result returns False for the example above, and result['KeY'] raisesKeyError`. Even basic functionality of the resulting fake identifier memory is completely broken.

[dgw]

Fix for pop() looks reasonable to me.

Fun fact: It wasn't reasonable, and in fact didn't raise KeyError if the key is missing and no default was provided.

Fixed that, and checked off some more of my task list w/fixes for .copy(), del memory['KeY'], and .get().

Still draft, though. I have several more behaviors to retest and then fix if necessary.

[dgw]

Think I'm about burned out on this for the day. But if my notes are correct, only __eq__()/__ne__() and setdefault() are left to check/unit-test/maybe-fix. 🎉

[dgw]

Turns out I lied about being done for the day. setdefault() was a pretty simple one, and then after toying with __eq__() for a bit I just decided not to bother.

Objects of different types probably should be considered unequal, unless there's a really good use-case I'm missing. (Anyone who really wants to force a comparison between a regular dict and a SopelIdentifierMemory can test identifier_memory == SopelIdentifierMemory(my_normal_dict) instead, since this patch fixes that behavior.)

[dgw]

Aha, need to decide what to do about those "assert statement has a side effect" errors from CodeQL. Now that I eliminated the intermediate variables from those tests, I don't really want to put them back. 😅

[Exirel]

Objects of different types probably should be considered unequal

In this case, absolutely, yes. A SopelIdentifierMemory is a very specific structure that has a very specific behavior. If people are going to do weird stuff with it, that weird stuff shouldn't work.

[dgw]

Welp, looks like no more CI builds on this branch until I rebase. Y'all mind if I squash out the fixups at the same time?

[Exirel]

Yes go ahead, this will fix the conflict too.

[dgw]

this will fix the conflict too

Yep, that's why I wanted to rebase. Hard to continue bikeshedding new test cases if CI won't run any more. 😝

Ran 3.10 & 3.8 tests locally; pushing in a moment, once I review the final history for rebase mistakes… I wound up doing three passes on it so there'd be less chance of putting fixups in the wrong order by accident, and I'm still not quite positive that I put everything in the right order. 😅

[dgw]

Got carried away and used git rebase --exec to run lint + tests on Python 3.11 for every commit in the cleaned-up history.

But because of that, I know every intermediate step works as expected and doesn't have any dangling imports or commits that were reordered incorrectly. :)

Edit: Why oh why did Python 3.8 linting fail? I specifically checked it before the whole --exec thing under 3.11! 😠

Edit 2: Fixed, of course. My linter fixup commit must have gotten lost somewhere in the rebase. Comparison locally of the old HEAD (ae6c337c9b8da4c3ac59a0502f1eb2e789ed70ae) vs. current HEAD shows that no other changes slipped. I must have screwed up a conflict resolution and pasted the old MemoryConstructorInput line on top of the fixed one.

[dgw]

I have another rebase in my future, apparently. Hopefully @SnoopJ can revisit the one or two open threads he started by the time Exi comes back from being "AFK" (as he said) for a few days, and I can rebase one last time with all the feedback/fixups then.

[dgw]

The fixups I just pushed (plus one technically unrelated new test I'm shoehorning in) catch even more edge cases that were missed, which @SnoopJ's nitpicks nudged me to find either directly or indirectly. Another very helpful review, thanks!

[dgw]

Latest force-push to squash out all the fixups moved one of the tests within the file, but meh. I made sure that the commit order is sane using git rebase master --exec '<pip3 deps> && <run lint/test>' and all the intermediate stages pass. I verified each commit's diff manually to be even more sure that the intermediate steps make logical sense. Can't imagine what else I could double check at this point, so let's :shipit:

Note: if you really really want a nitpick at this point, I still don't think it's necessary to do any assertion about how a builtin dict works.

I reduced the number of those, if not completely eliminated them. You're right that it isn't worth delaying the patch any longer.