Skip to content

Conversation

hartwork
Copy link
Contributor

@hartwork hartwork commented Oct 8, 2025

This mimics existing method SetReparseDeferralEnabled.
.. to make it consistent with the other four Expat security methods.
@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news labels Oct 8, 2025
@github-project-automation github-project-automation bot moved this to Todo in Docs PRs Oct 8, 2025
@hartwork 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.
@hartwork hartwork requested a review from picnixz October 9, 2025 13:36
@picnixz
Copy link
Member

picnixz commented Oct 9, 2025

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:

image

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
Copy link
Contributor Author

hartwork commented Oct 9, 2025

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:

note

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
Copy link
Contributor Author

hartwork commented Oct 9, 2025

@picnixz pushed.

@hartwork
Copy link
Contributor Author

@picnixz how do you like the new version?

@picnixz
Copy link
Member

picnixz commented Oct 13, 2025

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?

@hartwork hartwork requested a review from hugovk October 13, 2025 23:08
@hartwork
Copy link
Contributor Author

@hugovk thanks for the review! How about now?

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you!

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

hugovk commented Oct 14, 2025

And backport this to 3.13?

@picnixz
Copy link
Member

picnixz commented Oct 14, 2025

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
Copy link
Member

hugovk commented Oct 14, 2025

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
Copy link
Member

picnixz commented Oct 14, 2025

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
Copy link
Member

hugovk commented Oct 14, 2025

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

@hartwork
Copy link
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! 🥂

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