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
gh-83122: Deprecate testing element truth values in ElementTree
#31149
base: main
Are you sure you want to change the base?
gh-83122: Deprecate testing element truth values in ElementTree
#31149
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@gvanrossum I'm planning to merge this PR (you approved the idea on BPO a few years back)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question -- how likely is it that ElementTree is actually ever going to change this behavior? Hasn't it been super-stable for many years?
That's a fair point. On the bug report people were supportive of changing the behavior, but I don't know that it's ever actually going to happen. |
Let’s find out first. |
If the warning was only shown by the Python version, very few people have seen it, right, because (I presume) the C version is always used if it can be imported. I'm guessing that the idea is that nodes will always be truthy, so you can use |
Forgive me if this is stating the obvious, but as I see the status quo, users are tempted to write
I might be misunderstanding you, but since nodes are not always truthy, I'm hoping for ElementTree to discourage this usage.
The applications that need to change will be ones that desired
Right. Of course if the ultimate resolution here is a warning rather than an exception, we could just change |
Ah, I didn't put that very well. Of course applications will have to deal with the warning proposed in the issue (and in the python implementation). I meant something like, applications "that might find this change unwelcome". |
My understanding is that the warning put in the Python code (many, many releases ago?) was a FutureWarning because the plan of record at the time was to change the behavior, after a few releases of issuing warnings. (The error message says so: But since the warning is not printed in reality, there may also be code that has used the falsity of child-less nodes as a feature -- after all it was put in intentionally as a feature (long, long ago). Your experience of being bitten by this explains why it was a bad feature. It is not necessarily the case that code using I dislike warnings that live forever -- they have a place, but I don't have to like them, and if we're putting in a FutureWarning we promise to eventually change the behavior. So we need to have a think about that. |
I posted on the issue to make sure we gather consensus on the right way forward here. Until we do that I'm unassigning myself from this PR. Sorry for getting your hopes up @jacobtylerwalls! |
ElementTree
ElementTree
Lets change the warning in the existing code and the new equivalent added to the C code to DeprecationWarning. FutureWarning was never intended to be used for this purpose. If we go forward with this, it'll become a behavior change in >= 3.14. |
Modules/_elementtree.c
Outdated
element_bool(PyObject* self_) | ||
{ | ||
ElementObject* self = (ElementObject*) self_; | ||
(void)PyErr_WarnEx(PyExc_DeprecationWarning, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can return -1 to indicate that an error occurred (so something like if (PyErr_WarnEx(...) < 0) { return -1; }
. This is useful so that people who want to be strict about deprecations can turn this into an exception.
@@ -413,6 +413,11 @@ Deprecated | |||
is no current event loop set and it decides to create one. | |||
(Contributed by Serhiy Storchaka and Guido van Rossum in :gh:`100160`.) | |||
|
|||
* The :mod:`xml.etree.ElementTree` module now emits :exc:`DeprecationWarning` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should also be documentation in the docs for xml.etree.ElementTree
. Those docs should say when the deprecation is scheduled to be executed (3.14 I think) and what people should do instead when they encounter the deprecation in their code.
ElementTree
ElementTree
ElementTree
ElementTree
gh-83122
When testing element truth values, emit an identical
in the C implementation as in the Python implementation. EDIT: changed toFutureWarning
DeprecationWarning
in both implementations per comments.Matching an element in a tree search but having it test False can be unexpected. Raising the warning in both places enables making the choice to finally raise an exception for this ambiguous behavior in the future.
Documentation promising FutureWarnings:
Element.remove()
2.7 release notes
Fixes #83122