FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-90949: Recommend `hasattr` with Expat security methods by hartwork · Pull Request #139800 · python/cpython · GitHub

/ cpython Public

gh-90949: Recommend hasattr with Expat security methods - #139800

Merged
hugovk merged 9 commits into
python:mainfrom
hartwork:gh-90949-docs
Oct 14, 2025
Merged

gh-90949: Recommend hasattr with Expat security methods#139800
hugovk merged 9 commits into
python:mainfrom
hartwork:gh-90949-docs

Conversation

hartwork commented Oct 8, 2025
edited
Loading

Copy link
Copy Markdown
Contributor

This mimics existing method SetReparseDeferralEnabled.
.. to make it consistent with the other four Expat security methods.
bedevere-app Bot added docs Documentation in the Doc dir skip news labels Oct 8, 2025
github-project-automation Bot moved this to Todo in Docs PRs Oct 8, 2025
hartwork changed the title gh-90949: Recommend "hasattr" with Expat security methods gh-90949: Recommend hasattr with Expat security methods Oct 8, 2025
…otes

.. to make it consistent with Doc/library/pyexpat.rst.
Comment thread Doc/library/xml.etree.elementtree.rst Outdated
Comment thread Doc/library/xml.etree.elementtree.rst Outdated
Comment thread Doc/library/pyexpat.rst Outdated
hartwork requested a review from picnixz October 9, 2025 13:36

picnixz commented Oct 9, 2025

Copy link
Copy Markdown
Member

I think I'm fine with this addition. However, because we talk about "backported" interface, we shouldn't push this until it's effectively backported (at least once). I'm actually unsure about the expanded docs. For instance:

I feel that the "note" boxes are way too intrusive... I think it's more important to have the note about the surprising values rather than the note on the availability (the versionadded should be enough). OTOH, if you ever add more protections, we'll have a huge block of notes, almost always the same. Here's what I can suggest: a simple .. seealso:: which refers to a section about "Availability testing" which indicates that some features are included in micro versions instead of .0 versions. That would reduce the vertical span of the page and be more readable for users in general. WDYT?

hartwork commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

I think I'm fine with this addition. However, because we talk about "backported" interface, we shouldn't push this until it's effectively backported (at least once).

@picnixz I'm reading that as merging at least one of the unmerged backports of #139234 first — sure.

I feel that the "note" boxes are way too intrusive...

It's got a bit less loud in the meantime when adding the ! earlier — less bold and blue now:

I think it's more important to have the note about the surprising values rather than the note on the availability (the versionadded should be enough).

So there is a threshold where above a note gets its own box and below a box takes too much attention, makes sense.

OTOH, if you ever add more protections, we'll have a huge block of notes, almost always the same.

It would be multiple smaller blocks, not one huge block though. It's a bit less scary than that.

Btw I have no plans of adding more, new API like that will only appear if there is no way around it.

Here's what I can suggest: a simple .. seealso:: which refers to a section about "Availability testing" which indicates that some features are included in micro versions instead of .0 versions. That would reduce the vertical span of the page and be more readable for users in general. WDYT?

Personally I think that see also requires an additional click and additional energy and that alone will many users stop from ever noticing. (I also believe readability is not in danger currently.) Let me demo what we get when taking the .. note:: markers out and turning these notes back to text. Someone reading the whole method description will get the memo then, and it will not be as loud. Push coming up in a minute or two…

hartwork commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

@picnixz pushed.

Copy link
Copy Markdown
Contributor Author

@picnixz how do you like the new version?

picnixz commented Oct 13, 2025

Copy link
Copy Markdown
Member

I'm sorry but I don't have a lot of time to look at the Expat related issues/PRs. At first glance it looks fine (less visual distraction).

@hugovk As someone who has better insight on accessibility & co, what would you suggest here?

Comment thread Doc/library/pyexpat.rst Outdated
Comment thread Doc/library/pyexpat.rst Outdated
hartwork requested a review from hugovk October 13, 2025 23:08

Copy link
Copy Markdown
Contributor Author

@hugovk thanks for the review! How about now?

hugovk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you!

hugovk merged commit 0c17473 into python:main Oct 14, 2025
31 checks passed
github-project-automation Bot moved this from Todo to Done in Docs PRs Oct 14, 2025

hugovk commented Oct 14, 2025

Copy link
Copy Markdown
Member

And backport this to 3.13?

picnixz commented Oct 14, 2025

Copy link
Copy Markdown
Member

Actually, we didn't backported the methods to prior versions yet. I am currently starting my new work so I didn'tt have much time to focus on those PRs (currently, only main contains these new APIs)

hugovk commented Oct 14, 2025

Copy link
Copy Markdown
Member

OK, so should this not have been merged yet? It's only in the 3.15 docs for now, so not such a big deal if they're coming soon-ish.

picnixz commented Oct 14, 2025

Copy link
Copy Markdown
Member

Yes, I should have added the DO-NOT-MERGE, my fault (we discussed it in the comments but it should have been mentioned more explicitly). It's not really important as I plan to backport the PRs next week (but my work may take me some time) (half of them are ready, the other half isn't backported yet)

hugovk commented Oct 14, 2025

Copy link
Copy Markdown
Member

OK, let's leave this in main for now and do the backports later. Good luck with your new work!

Copy link
Copy Markdown
Contributor Author

OK, let's leave this in main for now and do the backports later.

@hugovk good plan, there is no real damage done, no worries.

Good luck with your new work!

From me as well! 🥂

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation in the Doc dir skip news

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL