Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#531 closed defect (fixed)

Python bindings attempt to use deprecated methods

Reported by: rq Owned by: Olly Betts
Priority: normal Milestone: 1.2.5
Component: Xapian-bindings (Python) Version: 1.2.4
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description (last modified by Olly Betts)

According to http://xapian.org/docs/deprecation.html, MSetItem.get_*() methods have been deprecated since 1.1. However, it seems like python-xapian itself attempts to make those calls when creating MSetItem[xapian.MSET_*] tuples, which causes an application attempting to use those tuples to err. See http://bugs.locamotion.org/show_bug.cgi?id=1766 for example. In this case, the Translate Toolkit did not use MSetItem.get_*() methods, but it still quit with an error when trying to use MSetItem[xapian.MSET_*]. Changing the appropriate lines to use MSetItem.* properties fixed the problem.

I suspect the real problem is in http://trac.xapian.org/browser/trunk/xapian-bindings/python/util.i#L195 and below, where MSetItem[xapian.MSET_*] tuples are being created. I'm no Python expert though, so I may be totally wrong here.

Change History (4)

comment:1 by Olly Betts, 13 years ago

Owner: changed from Richard Boulton to Olly Betts
Status: newassigned

The code in util.i which makes those calls is actually C++ code, and the get methods aren't deprecated in the C++ API, so that part is OK.

I think there is an issue here though - this worked in 1.0 but doesn't in 1.2, and isn't listed as deprecated:

for item in mset:
    print item[xapian.MSET_DID]

It wasn't actually documented as working in 1.0 though - the documented use of MSET_DID, etc is:

for item in mset.items:
    print item[xapian.MSET_DID]

(Note: mset.items not mset)

A better way to write this, which works in both 1.0 and 1.2 is:

for item in mset:
    print item.docid

Looking into this has turned up another issue - we didn't had a feature test for the MSET_* stuff. I've now added one, but it turns out MSET_DOCUMENT doesn't work, as the element of the tuple it refers to is never set!

My thoughts on how to address this mess:

  • Since the way you are using MSET_* was never documented as working, won't work in 1.2.0 to 1.2.4, and has an easy replacement which is a nicer API and works in both Xapian 1.0 and 1.2, I think it probably doesn't make sense to make it work again in 1.2.5 onwards. We should just list it in the deprecation document along with what to replace it with. This isn't ideal, but seems to be the best of the available options to me.
  • MSET_DOCUMENT has never worked it seems, so it doesn't make much sense to try to fix it now - we should just document that it has never worked.
  • The properties interface is nicer than the tuple one - I think we should just deprecate MSET_* (and eventually remove it at some future point).

comment:2 by Olly Betts, 13 years ago

Description: modified (diff)
Milestone: 1.2.5
Version: 1.2.4

OK, I've found out a little more...

Although not documented in the Python bindings HTML page, this way of using MSET_* was used by older versions of simpleindex.py (since 1.0.4 it's used the properties instead), so it is something that should have continued to work. Also, MSET_DOCUMENT worked in this case - mset[0][MSET_DOCUMENT] worked in 1.0.x, it's mset[0].items[MSET_DOCUMENT which has never worked.

But this isn't an API we've advertised at all since 1.0.4, which should at least reduce the number of users affected by this feature no longer working.

Richard agreed on IRC that documenting the issue and recommending people just use the properties instead was probably the best thing to do at this point.

Sorry about this - we try hard to keep the API stable and flag deprecations a whole release series in advance, but we messed up here.

I will update the deprecation document for 1.2.5 to say that:

  • mset[0][xapian.MSET_DID], etc were inadvertently removed in 1.2.0 and to use the property interface instead.
  • mset.items is now deprecated and to use the pythonic iterators and their property interface instead.
  • similarly, deprecate eset.items.

And I'll document that MSET_DOCUMENT isn't actually useful in 1.2.x.

If anyone has a better plan, please speak up.

comment:3 by Olly Betts, 13 years ago

Resolution: fixed
Status: assignedclosed

Python docs updated in r15345, and deprecation.rst in r15346, so closing now.

comment:4 by rq, 13 years ago

Thanks Olly!

Note: See TracTickets for help on using tickets.