Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-83122: Deprecate testing element truth values inElementTree#31149

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
gpshead merged 18 commits intopython:mainfromjacobtylerwalls:c-etree-bool-warning
Jan 23, 2023

Conversation

@jacobtylerwalls
Copy link
Contributor

@jacobtylerwallsjacobtylerwalls commentedFeb 5, 2022
edited
Loading

gh-83122

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

Copy link
Member

@JelleZijlstraJelleZijlstra left a 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)

Copy link
Member

@gvanrossumgvanrossum left a 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?

@JelleZijlstra
Copy link
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.

@gvanrossum
Copy link
Member

Let’s find out first.

@gvanrossum
Copy link
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 useif node as a shorthand (in most contexts) forif 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.

@jacobtylerwalls
Copy link
ContributorAuthor

Forgive me if this is stating the obvious, but as I see the status quo, users are tempted to writeif 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 thatdesiredif node to meanif node is not None and len(node), which seems like strange cases to commingle (especially in the negative, comminglingNone 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 changeFutureWarning toRuntimeWarning and be done with it. Then it could be something for linters to make an effort to detect and warn about.

@jacobtylerwalls
Copy link
ContributorAuthor

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".

@gvanrossum
Copy link
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 abad feature.

It is not necessarily the case that code usingif node meant it to meanif 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 meanif 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

@JelleZijlstra
Copy link
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

@jacobtylerwallsjacobtylerwalls changed the titlebpo-38941: Warn when testing element truth values in C version ofElementTreegh-83122: Warn when testing element truth values in C version ofElementTreeDec 11, 2022
@gpshead
Copy link
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

@jacobtylerwallsjacobtylerwalls changed the titlegh-83122: Warn when testing element truth values in C version ofElementTreegh-83122: Deprecate testing element truth values in C version ofElementTreeJan 21, 2023
@jacobtylerwallsjacobtylerwalls changed the titlegh-83122: Deprecate testing element truth values in C version ofElementTreegh-83122: Deprecate testing element truth values inElementTreeJan 21, 2023
@bedevere-bot
Copy link

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 phraseI 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.

@jacobtylerwalls
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@gpshead
Copy link
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

@gpsheadgpshead self-assigned thisJan 23, 2023
@gpsheadgpshead added stdlibStandard Library Python modules in the Lib/ directory extension-modulesC modules in the Modules dir and removed DO-NOT-MERGE labelsJan 23, 2023
Comment on lines +1463 to +1466
if (self->extra ?self->extra->length :0) {
return1;
}
return0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It could be simply

returnself->extra&&self->extra->length;

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@gvanrossumgvanrossumgvanrossum left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@gpsheadgpsheadgpshead approved these changes

@JelleZijlstraJelleZijlstraAwaiting requested review from JelleZijlstra

Assignees

@gpsheadgpshead

Labels

extension-modulesC modules in the Modules dirstdlibStandard 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

9 participants

@jacobtylerwalls@JelleZijlstra@gvanrossum@gpshead@bedevere-bot@serhiy-storchaka@iritkatriel@the-knights-who-say-ni@ezio-melotti

[8]ページ先頭

©2009-2025 Movatter.jp