bpo-32476 : Add concat function for ElementTree find by jjolly · Pull Request #5251 · python/cpython · GitHub

jjolly · 2018-01-20T17:53:33Z

Problem: Using apostrophes and quotes in a search string is impossible. Currently escaping quotes in the search string is not implemented.
Example:

>>> import xml.etree.ElementTree as et
>>> e = et.Element("root")
>>> c = et.SubElement(e, "branch")
>>> c.set("name", "main")
>>> gc = et.SubElement(c, "leaf")
>>> gc.set("name", "\"quote\" 'apostrophe'")
>>> et.tostring(e)
b'<root><branch name="main"><leaf name="&quot;quote&quot; \'apostrophe\'" /></branch></root>'
>>> r.findall(".//*[@name=\"&quot;quote&quot; 'apostrophe'\"]")
[]

Solution: This patch implements the XPath concat() function within an attribute search.
Result:

>>> r.findall(".//*[@name=concat('\"quote\"', ' ', \"'apostrophe'\")]")
[<Element 'leaf' at 0x7f8f9778cae8>]

https://bugs.python.org/issue32476

matrixise · 2018-01-30T09:38:38Z

maybe you could add a

..versionadded:: 3.7
    Support of the XPath concat function

jjolly · 2018-01-30T14:16:49Z

@matrixise How about full documentation on the new feature - including the versionadd?

matrixise · 2018-01-30T15:44:52Z

Fine, you have updated the documentation. Thank you

scoder · 2018-03-16T16:51:33Z

Lib/xml/etree/ElementPath.py

    r"\.\.|"
    r"\(\)|"
-    r"[/.*:\[\]\(\)@=])|"
+    r"[/.,*:\[\]\(\)@=])|"


Adding a comma here suggests that you should also add error tests for commas appearing in some unusual places.

There does not seem to be a precedence for such testing. Is there a similar test for unusually placed '@' or '=' tokens? I could implement a test such as these:

self.assertRaisesRegex(KeyError, ',', e.find, ',/tag[0]') self.assertRaisesRegex(KeyError, ',', e.find, '.,tag[0]') self.assertRaisesRegex(KeyError, ',', e.find, './,tag[0]') self.assertRaisesRegex(KeyError, ',', e.find, '.,/tag[0]')

If I added these tests, wouldn't it be necessary to add similar tests for the other characters in the regex? Do I test for every character in the regex? How far do I go?

Most of the other characters are part of the general tree structure and thus allowed in more places. All of these places are unlikely to be prepared for handling a ,, so this is a new case. I'd specifically recommend to add tests for things like ,[0], tag[,], tag[(,)] and so on.

I would generally say that covering the parser with more tests would be helpful, but you don't have to do that for this ticket. Since you're at it, though, feel free to add a couple of similar ones for =, : and @. You can do that in a loop, as long as the error message on failures is clear about what incorrect expression was tried.

scoder · 2019-02-19T06:40:04Z

Lib/xml/etree/ElementPath.py

"closing parenthesis"

scoder · 2019-02-19T06:40:30Z

Lib/xml/etree/ElementPath.py

"preceding"

scoder · 2019-03-04T13:23:54Z

One more thing: there should also be tests that it's still possible to look for tags and attributes called concat, i.e. that the parser is limited to special-casingconcat only where it's used as a function.

jjolly · 2019-05-15T18:48:02Z

One more thing: there should also be tests that it's still possible to look for tags and attributes called concat, i.e. that the parser is limited to special-casingconcat only where it's used as a function.

I have added a new tag and attributes in order to test concat in various places within the XML.

jjolly · 2019-05-15T18:55:39Z

...and the spelling changes. Sorry for the multiple pushes.

scoder · 2020-04-12T13:02:08Z

Lib/xml/etree/ElementPath.py

+        try:
+            token = next()
+        except StopIteration:
+            raise SyntaxError("missing closing parathesis for concat")


Suggested change

raise SyntaxError("missing closing parathesis for concat")

raise SyntaxError("missing closing parenthesis for concat")

scoder · 2020-04-12T13:22:59Z

Lib/test/test_xml_etree.py

+        self.assertEqual(e.find('./tag[@class=concat("d")]').attrib['class'], 'd')
+        self.assertEqual(e.find('./tag[@class=concat("", \'d\', "")]').attrib['class'], 'd')
+        self.assertEqual(e.find('./tag[@value="concat"]').attrib['class'], 'b')
+        self.assertEqual(e.find('./tag[@value=concat("concat")]').attrib['class'], 'b')
+        self.assertEqual(e.find('./tag[@concat="value"]').attrib['class'], 'd')
+        self.assertEqual(e.find('./tag[@concat=concat("value")]').attrib['class'], 'd')
+        self.assertEqual(e.find('./concat[@class=concat("x")]').attrib['class'], 'x')


Ok, but … none of these tests represents a real use case for concat(), does it? What about concat("prefix-", @attr) or concat("'", ., "'") ? Those seem to be the really interesting cases to me.

I understand that your intention was to allow for escaping, but a) I can't see that being covered by the tests and b) does that really make a use case for concat() already?

My fear is that we're opening up a feature here which users will bump into and be disappointed because it doesn't actually do what they want. It really just serves the tiny use case of concatenating string constants right now. That's … unexpected at best.

MaxwellDupre · 2022-05-08T11:06:45Z

Could you amend versionchanged, please? Lets try for 3.11.

github-actions · 2022-08-14T00:12:27Z

This PR is stale because it has been open for 30 days with no activity.

scoder · 2022-08-14T09:02:09Z

Closing since the implementation isn't complete while the solution is not targeted to the specific problem. It seems that we'd add more expectations than solutions here.

the-knights-who-say-ni added the CLA signed label Jan 20, 2018

bedevere-bot added the awaiting review label Jan 20, 2018

scoder reviewed Mar 16, 2018

View reviewed changes

jjolly force-pushed the etree_xpath_concat branch from cb286b0 to 67c0d33 Compare February 19, 2019 04:56

scoder reviewed Feb 19, 2019

View reviewed changes

jjolly force-pushed the etree_xpath_concat branch from 67c0d33 to 8128317 Compare May 15, 2019 18:39

jjolly added 3 commits May 15, 2019 11:45

bpo-32476: Add concat function for ElementTree find

c5292e8

bpo-32476: Add testing for XPath concat search

114b264

bpo-32476: Add documentation for XPath concat search

6a4c530

jjolly force-pushed the etree_xpath_concat branch from 8128317 to 3d714fa Compare May 15, 2019 18:46

jjolly force-pushed the etree_xpath_concat branch from 3d714fa to 75dff0a Compare May 15, 2019 18:55

bpo-32476: Clarified xpath concat error messages

67a1c19

jjolly force-pushed the etree_xpath_concat branch from 75dff0a to 67a1c19 Compare May 15, 2019 20:03

csabella requested a review from scoder February 7, 2020 12:20

scoder reviewed Apr 12, 2020

View reviewed changes

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 14, 2022

scoder closed this Aug 14, 2022

	raise SyntaxError("missing closing parathesis for concat")
	raise SyntaxError("missing closing parenthesis for concat")

Uh oh!

Conversation

jjolly commented Jan 20, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matrixise commented Jan 30, 2018

Uh oh!

jjolly commented Jan 30, 2018

Uh oh!

matrixise commented Jan 30, 2018

Uh oh!

scoder Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

jjolly Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

scoder Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

scoder Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

scoder Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

scoder commented Mar 4, 2019

Uh oh!

jjolly commented May 15, 2019

Uh oh!

jjolly commented May 15, 2019

Uh oh!

scoder Apr 12, 2020

Choose a reason for hiding this comment

Uh oh!

scoder Apr 12, 2020

Choose a reason for hiding this comment

Uh oh!

MaxwellDupre commented May 8, 2022

Uh oh!

github-actions bot commented Aug 14, 2022

Uh oh!

scoder commented Aug 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jjolly commented Jan 20, 2018 •

edited by bedevere-bot

Loading