memories: test/fix interactions between SopelIdentifierMemory ↔ dict
#2525
Conversation
dgw
added
Medium Priority
Bugfix
dgw
force-pushed
the
SopelIdentifierMemory-casts
branch
from
f594db6
to
99685d1
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dgw
force-pushed
the
SopelIdentifierMemory-casts
branch
2 times, most recently
from
079df7a
to
dd14707
Compare
Fun fact: It wasn't reasonable, and in fact didn't raise Fixed that, and checked off some more of my task list w/fixes for Still draft, though. I have several more behaviors to retest and then fix if necessary. |
dgw
force-pushed
the
SopelIdentifierMemory-casts
branch
from
a2038d1
to
1d5ecea
Compare
|
Think I'm about burned out on this for the day. But if my notes are correct, only |
|
Turns out I lied about being done for the day. 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 |
dgw
changed the title
SopelIdentifierMemory and regular dict
dgw
changed the title
SopelIdentifierMemory and regular dictSopelIdentifierMemory ↔ dict
|
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. 😅 |
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. |
|
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? |
|
Yes go ahead, 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
force-pushed
the
SopelIdentifierMemory-casts
branch
from
ae6c337
to
7c48831
Compare
dgw
force-pushed
the
SopelIdentifierMemory-casts
branch
2 times, most recently
from
353e3d0
to
e546b8e
Compare
There was a problem hiding this comment.
Alright, that sound pretty good so far.
Aside from various recommendations or requests, my repeating points are:
- you can use
memory[key] == dict[key]to check that the value is the same/expected when you create from dict or by using copy - check that a copy doesn't alter the original, and vice versa
And I agree with SnoopJ on the left over "key not in memory", they don't look relevant.
|
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. |
|
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! |
There was a problem hiding this comment.
I'm going to be honest: the tests look good. The code looks good. Is there anything missing? Maybe, but I can't tell. All I know is that it looks very thorough, and if a bug arises... then we'll fix it. I don't see anything that would prevent us from fixing any issue, as the design sounds sane to me, and that's the most important part.
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. But it's already here and it works, so there is no argument to have here.
Resolves `KeyError` currently raised if the `key` parameter (`str`) doesn't match the internal (lowercased) representation of `Identifier`. That is, `del an_identifier_memory['key']` might have worked, but doing `del an_identifier_memory['KeY']` would not. This is now fixed.
Calling `.copy()` on a `SopelIdentifierMemory` now returns the correct type of object (`SopelIdentifierMemory`, instead of a plain `dict`). Co-authored-by: SnoopJ <[email protected]>
Co-authored-by: SnoopJ <[email protected]>
Co-authored-by: SnoopJ <[email protected]>
I'm timid about changing the actual method signature for `__init__()`, but could be talked into letting Python itself raise the `TypeError` for us if this approach looks too dumb. (All of the memory types can be cleaned up at once later, which is another reason I chose not to change the signature for just one of them.) But here we are: `SopelIdentifierMemory.__init__()` now handles being passed an iterable of `(key, value)` 2-tuples (whether the iterable itself is a list or a tuple), a regular `dict`, and an existing `SopelIdentifierMemory` object. In that last case, it's doing some unnecessary work in most cases by always casting the keys to whatever the `identifier_factory` function returns, but there are *probably* a few obscure cases where that might actually be useful behavior so I'll leave it.
Constructing a `SopelMemory` by passing kwargs as key-value pairs is not currently supported. This applies to all derived types, and since there are now a bunch of tests for `SopelIdentifierMemory`, it gets to be the guinea pig for an xfail-marked test of that fact. The same limitation applies to `SopelMemoryWithDefault`, since it also shares `SopelMemory`'s constructor method signature (which does not accept any kwargs). Co-authored-by: Exirel <[email protected]>
Prior to some of the recent work done on this class, calling `.clear()` on a `SopelIdentifierMemory` instance would turn that object into a normal `dict`, according to my tests a little while ago. That appears to no longer be the case, and though I didn't explicitly work on that problem yet, a unit test to make sure the behavior isn't broken again will do us good.
Comes with a slight bonus refactoring to share the argument-processing code previously inlined in `__init__()` with `update()`. Co-authored-by: SnoopJ <[email protected]>
New `|` / `|=` operators added in Python 3.9 for dicts. However, because we're now defining their dunder methods, they will also work in 3.8 for SopelIdentifierMemory objects. Co-authored-by: SnoopJ <[email protected]> Co-authored-by: Exirel <[email protected]>
Comparing two `SopelIdentifierMemory` instances using `==` already worked, but if one side of the comparison was a plain `dict` it would be almost impossible for the two to be considered equal. It's obviously doable to override `__eq__()`/`__ne__()` and make the two types test as equal if they have equal values associated with keys-that- are-equivalent-when-compared-as-`Identifier`s, but we probably shouldn't do that. I'm perfectly happy to consider objects of different types as "not equal" even if they contain equivalent key-value pairs. Therefore, these new tests codify the `==`/`!=` behavior as follows: * `SopelIdentifierMemory` objects will compare as equal if their keys and values compare as equal, even if they use different `IdentifierFactory` (the `make_identifier` parameter) implementations. * A `SopelIdentifierMemory` and a `dict[Identifier]` MAY compare as equal if the `Identifier` keys in each were created by compatible (but not necessarily identical) `IdentifierFactory` implementations. * A `SopelIdentifierMemory` and a `dict[str]` (or any other `dict` with non-`Identifier` keys) are not expected to compare as equal.
Since the whole point of this type is to have a thread-safe version of `collections.defaultdict` and not have to check for `'key' in SMWD` before accessing something, the `__contains__()` method (supporting the `in` keyword) is never exercised in regular code. The rest of `SopelMemoryWithDefault` is covered by other test cases for code that *uses* the type, so I'll just keep this patch small. Now the only uncovered code in `sopel/tools/memories.py` is for variadic argument handling that we plan to remove anyway, so 98% is fine for now.
dgw
force-pushed
the
SopelIdentifierMemory-casts
branch
from
11ea3a8
to
a463d16
Compare
|
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
I reduced the number of those, if not completely eliminated them. You're right that it isn't worth delaying the patch any longer. |
Description
Tin. Will resolve #2524.
Checklist
make qa(runsmake lintandmake test)Notes
Started as a partial, untested fix from my tablet to reserve the PR number. New day, new task list:
del d['KeY']d.clear()(appears to work, but the object becomes a regulardictand loses theIdentifiermagic)d.copy()Made a copy
mand added another item to it, verifying that the new item did not appear in the originald.However, the new object
mis adict, not aSopelIdentifierMemory.d.get('KeY')Returns the default passed to
.get(), notkey's value.d.pop('KeY')(currently raisesKeyError)d.pop('KeY', 'default')(currently always returns'default')d.pop('NotInDict')Returns
Nonebut should raiseKeyError.d.setdefault('KeY', 'default')With
'key' in d, returns'default'& adds'KeY'todas a regular string, notIdentifier.d.update(a_plain_dict)If
dhasIdentifier('thing1')as a key, anda_plain_dicthas'ThinG1'(a regular string), they will not be merged correctly anddwill now also have a regular string key called'ThinG1'.d | a_plain_dictSame problem as
d.update(a_plain_dict).d |= a_plain_dictSame problem as
d.update(a_plain_dict)andd | a_plain_dict.[ ]d == a_plain_dictIf
a_plain_dicthas the same key/value pairs but the key capitalization doesn't matchIdentifier's canonical (lowercase) internal representation, the equality check returnsFalse.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 😁).
result = SopelIdentifierMemory({'KeY': 'foo'})resultis of typeSopelIdentifierMemory, but without the magic. Existing keys are not converted toIdentifier, and'key' in resultdoesn't work as expected for that type.In fact, even
'KeY' in resultreturnsFalsefor the example above, andresult['KeY'] raisesKeyError`. Even basic functionality of the resulting fake identifier memory is completely broken.