Opened 15 years ago

Closed 15 years ago

#2502 enhancement closed fixed (fixed)

'and' and 'or' operator suport for xpath

Reported by: jack Owned by:
Priority: highest Milestone: Words-0.6
Component: words Keywords:
Cc: twonds, Ralph Meijer, Jean-Paul Calderone Branch:
Author:

Description

The attached patch (with updated and addition test cases) adds 'and' and 'or' operators to the xpath grammar. Now xpath predicate expressions can be more complex (example: /iq[@from='someone@…/cpc' and (@type='set' or @type='get')]).

This makes programming with twisted words much easier as callback functions can be made very specific, instead of having to have routing functions doing the extra work.

Also, writing test cases is significantly easier, since you can use the xpath matcher to do much of the error checking work.

The patch is very simple, but there are lots of minor doc edits because someone had not been keeping the grammar file updated (they were just changing the generated output).

Attachments (1)

xpath.diff (22.1 KB) - added by jack 15 years ago.
new operators for xpath

Download all attachments as: .zip

Change History (16)

comment:1 Changed 15 years ago by jack

Oops. There was a small bug in the grammar that made three operands not work. I've modified the patch to test for this and fixed the problem.

Changed 15 years ago by jack

Attachment: xpath.diff added

new operators for xpath

comment:2 Changed 15 years ago by Ralph Meijer

Owner: changed from Jean-Paul Calderone to Ralph Meijer

comment:3 Changed 15 years ago by Ralph Meijer

Keywords: review added; words xpath xish removed
Milestone: Words-0.6
Owner: Ralph Meijer deleted
Priority: normalhighest

I created a branch source:branches/xish-xpathparser-2502 and imported Jack's patch there. I reviewed the actual changes and tests and they look fine. Thanks!

Because I wasn't happy with how xpathparser.py was generated, I included the Yapps runtime in a separate module, added comments on how the generated code file should not be edited and did some general cleanup.

Please review.

comment:4 Changed 15 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: set to Ralph Meijer

The granularity of test_staticMethods in test_xpath.py doesn't seem right. The "static methods" of xpath aren't a unit - probably xpath.matches is at least one, xpath.queryForNodes is at least one, xpath.queryForString is at least one, and xpath.queryForStringList is at least one. The vagueness of the docstring somewhat reflects this - if the test had more direction, it would probably be easier to describe. Other tests in this module have similar problems. The size of the document on which they're all operating also suggests they're doing too much - few of the tests need more than 3 or 4 nodes to cover the behavior they're trying to cover, but they're all operating on a document with more than 20 nodes. This is really a pre-existing issue with the code, but I'm pointing it out since a lot of tests have minor changes made to them in this branch, and some new tests are added in the same style.

Thanks for making the status of xpathparser.py more explicit :) One change I'd suggest is making the yappsrt import absolute. That is, from twisted.words.xish import yappsrt as runtime.

In xpath.py, BooleanValue should document its instance variables and _booleanAnd and _booleanOr should probably have docstrings.

comment:5 Changed 15 years ago by Ralph Meijer

Keywords: review added
Owner: changed from Ralph Meijer to Jean-Paul Calderone

Yes those tests do not deserve any price for aesthetics. That should be changed in the future. Deferring that now.

As for the other comments, those have been addressed in the branch. I suppose an Interface description would be a good idea for better documentation down the line.

Please review.

comment:6 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Ralph Meijer

Looks good, go ahead and merge.

comment:7 Changed 15 years ago by Ralph Meijer

Resolution: fixed
Status: newclosed

(In [20496]) Add and and or operators for Xish XPath expressions.

Author: jack (ralphm) Reviewer: ralphm, exarkun Fixes #2502.

This change also uses the separate yapps runtime, simplifying future changes to the grammar.

comment:8 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

Reverted at r20528:

  • doc buildslave has failures - let's fix these by pulling yappsrt out of Twisted completely, I think that'll resolve the issue
  • the generated xpathparser.py also has manual modifications, which we should avoid :)

comment:9 Changed 15 years ago by Ralph Meijer

For reference, this failure on the doc buildslave is covered in #2701.

comment:10 Changed 15 years ago by Jean-Paul Calderone

For the record, the code for this ticket is still in xish-xpathparser-2502. The yapps code has been moved out of twisted/words/. If we want to distribute it, then it seems like the thing to do is to put it into twisted/words/topfiles and adjust the setup.py to install it.

comment:11 Changed 15 years ago by Ralph Meijer

Keywords: review added
Owner: changed from Ralph Meijer to Jean-Paul Calderone
Status: reopenednew

Modified source:branches/xish-xpathparser-2502-2 two weeks ago to reembed the yapps runtime and better document the generation process.

Please review again.

comment:12 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Ralph Meijer

The sed expression seems to be slightly buggy ;)

# 3.) Edit the output to depend on the embedded runtime, not 

There's also some trailing whitespace in xpathparser.g, which I suppose should be cleaned up.

The rest seems kosher.

comment:13 Changed 15 years ago by Ralph Meijer

Heh. Oops. Fixed and committed. Merging.

comment:14 Changed 15 years ago by Ralph Meijer

Resolution: fixed
Status: newclosed

(In [20726]) Add and and or operators for Xish XPath expressions.

Author: jack (ralphm) Reviewer: ralphm, exarkun Fixes #2502.

comment:15 Changed 11 years ago by <automation>

Owner: Ralph Meijer deleted
Note: See TracTickets for help on using tickets.