search

zopefoundation / zope.index

Indices for using with catalog like text, field, etc.
Other
10 stars 12 forks source link

handle None value in field index #12

Closed fgregg closed 7 years ago

fgregg commented 7 years ago

This should fix a test that depended upon None not being an acceptable key in Btree, which is no longer valid for current versions of BTree.

jamadden commented 7 years ago

Interestingly, when the now-failing test was first added back in https://github.com/zopefoundation/zope.index/commit/9c0933df1095c0ee4d8258350034734ac5b32043, the if value is not None line was maintained (indeed, there was one more 'is not None' check at the beginning of the method). (That line was added in https://github.com/zopefoundation/zope.index/commit/d1f299abea1e87a215e81b6369fb974e90388dd1, but without tests, presumably to workaround this BTrees 4 issue, on the same day.) It was with a subsequent commit https://github.com/zopefoundation/zope.index/commit/43e8fb80bae84cecad3455c604b5594d0a78baa2 also on that same day that the if check was removed in favor of catching the TypeError.

Given that the original behaviour under BTrees 3 was to allow None keys, and that the restoration of that behaviour recently was for the benefit of the Plone community (who, IIUC, do want None keys in Products.ZIndex(?)), I'm wondering if the right thing to do might be to go back to the code pre- 9c0933df1095c0ee4d8258350034734ac5b32043? That is, remove both the 'if' and 'try' statements (and alter the tests)? That would allow None keys, while not silently hiding TypeError for other invalid keys.

jamadden commented 7 years ago

The OS X 'homebrew 3' build failed. It has switched to Python 3.6, and apparently we don't support Python 3.6 yet (the linux build is pinned to 3.5 max). This has been dealt with before in BTrees and others and probably needs to change here too (although probably in a separate PR).

fgregg commented 7 years ago

@jamadden this follows your suggestion.

jamadden commented 7 years ago

It LPGTM and it makes sense to me, but then again it was my suggestion 😄 Plus I don't actually have any commits in this repo. So I don't feel like I should be the sole approver myself. Maybe someone with the most commits (i.e., @tseaver )? Also, @mgedmin brought it up in #9

jamadden commented 7 years ago

The fact that this requires BTrees > 4.4.1, plus the fact that the try/except that hid TypeError for really incomparable keys is gone probably deserves a CHANGES mention.

fgregg commented 7 years ago

@tseaver @jamadden I think this is good to go.

jamadden commented 7 years ago

It still LGTM, and I believe the comment @tseaver made has been addressed. I think you can hit the merge button 😄 (Then I'll take a look at #8 since the builds are passing again.)