GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-83122: Deprecate testing element truth values in `ElementTree` by jacobtylerwalls · Pull Request #31149 · python/cpython

Conversation

@jacobtylerwalls

@jacobtylerwalls jacobtylerwalls commented Feb 5, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

gh-83122

When testing element truth values, emit an identical FutureWarning in the C implementation as in the Python implementation. EDIT: changed to 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jacobtylerwalls
bpo-38941: Warn when testing element truth values in C version of Ele…
acd875f
…mentTree
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 5, 2022
@bedevere-bot bedevere-bot added the awaiting review label Feb 5, 2022
jacobtylerwalls added 2 commits February 5, 2022 16:04
@jacobtylerwalls
Fix bug in implementation
7e0be26
@jacobtylerwalls
Merge branch 'main' into c-etree-bool-warning
f4738d2
JelleZijlstra
JelleZijlstra requested changes Feb 13, 2022
View reviewed changes
Comment thread Modules/_elementtree.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_xml_etree_c.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Feb 13, 2022
jacobtylerwalls added 2 commits February 13, 2022 09:15
@jacobtylerwalls
Move test to Python suite
331a577
@jacobtylerwalls
Concision
87e8ad7
@jacobtylerwalls
jacobtylerwalls requested a review from JelleZijlstra March 5, 2022 14:18
JelleZijlstra
JelleZijlstra approved these changes Mar 5, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Mar 5, 2022
@JelleZijlstra JelleZijlstra self-assigned this Mar 5, 2022
gvanrossum
gvanrossum reviewed Mar 5, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@JelleZijlstra

JelleZijlstra commented Mar 5, 2022

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Mar 5, 2022

Copy link
Copy Markdown
Member

Let’s find out first.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Mar 5, 2022

Copy link
Copy Markdown
Member

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 if node as a shorthand (in most contexts) for if node is not None right? I do endorse that in principle, but I'm not sure if it's worth requiring all elementtree applications to change.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jacobtylerwalls

jacobtylerwalls commented Mar 5, 2022

Copy link
Copy Markdown
Contributor Author

Forgive me if this is stating the obvious, but as I see the status quo, users are tempted to write if node under the mistaken belief that all nodes are truthy, and then get bitten when the condition excludes childless nodes. I made this mistake a few times when learning ElementTree, and it's something I have to remember to look for in code review. I would have much preferred to see the warning! :-)

I'm guessing that the idea is that nodes will always be truthy, so you can use if node as a shorthand (in most contexts) for if node is not None right?

I might be misunderstanding you, but since nodes are not always truthy, I'm hoping for ElementTree to discourage this usage.

I'm not sure if it's worth requiring all elementtree applications to change.

The applications that need to change will be ones that desired if node to mean if node is not None and len(node), which seems like strange cases to commingle (especially in the negative, commingling None and childless nodes).

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.

Right. Of course if the ultimate resolution here is a warning rather than an exception, we could just change FutureWarning to RuntimeWarning and be done with it. Then it could be something for linters to make an effort to detect and warn about.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jacobtylerwalls

jacobtylerwalls commented Mar 5, 2022

Copy link
Copy Markdown
Contributor Author

The applications that need to 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".

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Mar 6, 2022

Copy link
Copy Markdown
Member

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: r"The behavior of this method will change in future versions. ")

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 if node meant it to mean if node is not None and len(node) -- the code could already know that node is an elementtree node, and the user could have meant it to mean if len(node).

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.

jacobtylerwalls reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra JelleZijlstra added the DO-NOT-MERGE label Mar 23, 2022
@JelleZijlstra JelleZijlstra removed their assignment Mar 23, 2022
@JelleZijlstra

JelleZijlstra commented Mar 23, 2022

Copy link
Copy Markdown
Member

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!

jacobtylerwalls reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@iritkatriel iritkatriel removed the awaiting merge label Nov 6, 2022
@jacobtylerwalls jacobtylerwalls changed the title bpo-38941: Warn when testing element truth values in C version of ElementTree gh-83122: Warn when testing element truth values in C version of ElementTree Dec 11, 2022
@philiprowlands philiprowlands mannequin mentioned this pull request Jan 11, 2023
xml.etree.ElementTree.Element inconsistent warning for bool #83122
Closed
@gpshead

gpshead commented Jan 12, 2023

Copy link
Copy Markdown
Member

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.

JelleZijlstra, gvanrossum, and jacobtylerwalls reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jacobtylerwalls jacobtylerwalls changed the title gh-83122: Deprecate testing element truth values in C version of ElementTree gh-83122: Deprecate testing element truth values in ElementTree Jan 21, 2023
@jacobtylerwalls
Merge branch 'main' into c-etree-bool-warning
e8bf37d
gpshead
gpshead requested changes Jan 22, 2023
View reviewed changes
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_elementtree.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/xml.etree.elementtree.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting changes label Jan 22, 2023
@bedevere-bot

bedevere-bot commented Jan 22, 2023

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jacobtylerwalls added 3 commits January 22, 2023 12:55
@jacobtylerwalls
Avoid mentioning messy history
3223126
@jacobtylerwalls
Adjust warning message and stacklevel
22a1618
@jacobtylerwalls
Merge branch 'c-etree-bool-warning' of https://github.com/jacobtylerw…
447d738
…alls/cpython into c-etree-bool-warning
@jacobtylerwalls

jacobtylerwalls commented Jan 22, 2023

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Jan 22, 2023
@bedevere-bot

bedevere-bot commented Jan 22, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

@JelleZijlstra, @gpshead: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested review from JelleZijlstra and gpshead January 22, 2023 18:40
gpshead
gpshead approved these changes Jan 23, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Jan 23, 2023
@gpshead
gpshead merged commit d717be0 into python:main Jan 23, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jan 23, 2023
@gpshead

gpshead commented Jan 23, 2023

Copy link
Copy Markdown
Member

thanks! I'm so glad to see this long documented as a "bad idea" API wart on the way towards being cleaned up! :)

jacobtylerwalls reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead gpshead self-assigned this Jan 23, 2023
@gpshead gpshead added stdlib Standard Library Python modules in the Lib/ directory extension-modules C modules in the Modules dir and removed DO-NOT-MERGE labels Jan 23, 2023
serhiy-storchaka
serhiy-storchaka reviewed Jan 23, 2023
View reviewed changes
Comment thread Modules/_elementtree.c
Comment on lines +1463 to +1466
if (self->extra ? self->extra->length : 0) {
return 1;
}
return 0;

@serhiy-storchaka serhiy-storchaka Jan 23, 2023

Copy link
Copy Markdown
Member

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.

It could be simply

return self->extra && self->extra->length;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jacobtylerwalls
jacobtylerwalls deleted the c-etree-bool-warning branch March 26, 2023 02:25
@jacobtylerwalls jacobtylerwalls mentioned this pull request Nov 19, 2023
Avoid testing truth value of ElementTree.Element cuthbertLab/music21#1666
Merged
@jacobtylerwalls jacobtylerwalls mentioned this pull request May 26, 2024
Adjust deprecation warning re: testing truth value of xml.etree.ElementTree.Element #119577
Closed
@TomiBelan TomiBelan mentioned this pull request Jun 18, 2026
Forbid lxml.etree.Element.__bool__? abelcheung/types-lxml#126
Open
@austinmatherne-wk austinmatherne-wk mentioned this pull request Oct 26, 2026
Don't truth test lxml elements Arelle/Arelle#2701
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@gpshead gpshead gpshead approved these changes
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

extension-modules C modules in the Modules dir stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

xml.etree.ElementTree.Element inconsistent warning for bool

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@jacobtylerwalls @JelleZijlstra @gvanrossum @gpshead @bedevere-bot @serhiy-storchaka @iritkatriel @the-knights-who-say-ni @ezio-melotti
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.