gh-123963: Expose GetCurrentByteCount from expat #123964
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Thanks for the PR!
Please, add a few notes about this feature in Doc/whatsnew/3.14.rst.
Additionally, as far as I can see, there should be more changes, take a look at the XML_GetCurrentLineNumber. I think we should something similar for XML_GetCurrentByteCount
Misc/NEWS.d/next/Library/2024-09-11-16-40-12.gh-issue-123963.TzzIY8.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2024-09-11-16-40-12.gh-issue-123963.TzzIY8.rst
Outdated
Show resolved
Hide resolved
| @@ -506,6 +506,7 @@ def EndElementHandler(self, name): | |||
| def check_pos(self, event): | |||
| pos = (event, | |||
| self.parser.CurrentByteIndex, | |||
| self.parser.CurrentByteCount, | |||
There was a problem hiding this comment.
Should we have a dedicated test as well for this one? The libexpact docs say:
Returns 0 if the event is inside a reference to an internal entity and for the end-tag event for empty element tags (the later can be used to distinguish empty-element tags from empty elements using separate start and end tags
There was a problem hiding this comment.
The test case already includes an example of an empty-element as defined in the XML spec1. What is missing is an example of an empty element in the long form (with a separate open and close tag), but that seems redundant.
I'll gladly add it if you'd like me to explicitly include it.
Is the consensus that this change it large enough to warrant an item in the what's new? I don't mean to be a busybody, but the verbiage of the contribution guide says:
Does this qualify as that? I'll gladly copy it over, but I want to make sure I'm not creating more work than I'm contributing here ;) |
DelusionalLogic
force-pushed
the
main
branch
from
ad68318
to
e3ec869
Compare
|
I went ahead and added the changes requested in the review. The test comment is still open, since I'm awaiting some feedback on that. There also seems to be some problem with the Docs part of the build, something about the reference target of :attr:xmlparser.CurrentByteCount not being found. I don't think I understand enough about the documentation subsystem to really do anything intelligent here :/ |
When you're trying to preserve the format for the XML, it can be handy to know the length of the input that generated the event. Expat already has the feature (and seems to have had it for a long while), so why not expose it?
I've added a new condition to the test that was already added for the other GetCurrent* methods.
📚 Documentation preview 📚: https://cpython-previews--123964.org.readthedocs.build/