Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-39495: Remove default value from C impl of TreeBuilder.start #18275

Open
wants to merge 2 commits into
base: master
from

Conversation

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Jan 30, 2020

@hauntsaninja

This comment has been minimized.

Copy link
Contributor Author

hauntsaninja commented Jan 30, 2020

As suggested by Serhiy, this changes the C implementation of TreeBuilder.start to remove the default value.

Note however, that the C implementation of TreeBuilder.start still accepts None as a value whereas the Python implementation TypeErrors (see snippet at end).

I looked into changing the C implementation to also throw a TypeError. Unfortunately, it appears treebuilder_handle_start is called elsewhere where attrib can be Py_None (set on line 3310) and I'm not familiar enough with this module to know if I can break that:

res = treebuilder_handle_start((TreeBuilderObject*) self->target,

~/dev/cpython bpo39495 λ ./python.exe                       
Python 3.9.0a3+ (heads/bpo39495_2-dirty:2e6569b669, Jan 29 2020, 22:45:33) 
[Clang 11.0.0 (clang-1100.0.33.17)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import xml.etree.ElementTree
>>> # show the fix
>>> xml.etree.ElementTree.TreeBuilder().start("asdf")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: start expected 2 arguments, got 1
>>> # function still accepts None
>>> xml.etree.ElementTree.TreeBuilder().start("asdf", None)
<Element 'asdf' at 0x10b2e6710>

vs

~ λ python3.9
Python 3.9.0a3+ (heads/master:188bb5b, Jan 29 2020, 22:55:03) 
[Clang 11.0.0 (clang-1100.0.33.17)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import xml.etree.ElementTree
>>> # not fixed
>>> xml.etree.ElementTree.TreeBuilder().start("asdf")
<Element 'asdf' at 0x10b89f2c0>
>>> # function accepts None
>>> xml.etree.ElementTree.TreeBuilder().start("asdf", None)
<Element 'asdf' at 0x10b89f310>
>>> from test.support import import_fresh_module  
>>> pyElementTree = import_fresh_module('xml.etree.ElementTree', blocked=['_elementtree'])
>>> # show the inconsistency with python implementation
>>> pyElementTree.TreeBuilder().start("asdf")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: start() missing 1 required positional argument: 'attrs'
>>> # function does not accept None, throws TypeError
>>> pyElementTree.TreeBuilder().start("asdf", None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/shantanu/.pyenv/versions/3.9-dev/lib/python3.9/xml/etree/ElementTree.py", line 1463, in start
    self._last = elem = self._factory(tag, attrs)
  File "/Users/shantanu/.pyenv/versions/3.9-dev/lib/python3.9/xml/etree/ElementTree.py", line 171, in __init__
    raise TypeError("attrib must be dict, not %s" % (
TypeError: attrib must be dict, not NoneType
@corona10 corona10 requested a review from serhiy-storchaka Feb 1, 2020
@@ -0,0 +1 @@
Remove default value from ``attrs`` parameter of ``xml.etree.ElementTree.TreeBuilder.start`` for consistency between Python and C implementations.

This comment has been minimized.

Copy link
@corona10

corona10 Feb 1, 2020

Member
Suggested change
Remove default value from ``attrs`` parameter of ``xml.etree.ElementTree.TreeBuilder.start`` for consistency between Python and C implementations.
Remove default value from *attrs* parameter of :meth:`xml.etree.ElementTree.TreeBuilder.start` for consistency between Python and C implementations.
@serhiy-storchaka

This comment has been minimized.

Copy link
Member

serhiy-storchaka commented Feb 1, 2020

Thank you for your PR @hauntsaninja.

I prefer to remove also the support of None as a value of attrib in treebuilder_handle_start. According to the documentation attrib is a dict, and a dict is always passed to custom TreeBuilder.start().

treebuilder_handle_start is called from two places: the implementation of TreeBuilder.start() and expat_start_handler. In the latter case None is passed for optimization, to avoid creation of an empty dictionary. We can use NULL instead of None for this optimization.

@serhiy-storchaka

This comment has been minimized.

Copy link
Member

serhiy-storchaka commented Feb 1, 2020

Or you can just use object(subclass_of='&PyDict_Type') for attrs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.