Add docstrings relating to functions involving parser context to parser.pxi #449

abepolk · 2025-01-29T21:51:14Z

I wrote docstrings for a number of functions in parser.pxi. Since the docs said parser.pxi is "definitely not the right place to start reading lxml's source code," I thought it would be helpful to make it more accessible to newcomers.

In particular, I focused on functions that involved the parser context. This includes the context at the lxml/Cython level and the libxml2/C level. I wrote these based on notes I took as I was reading through parser.pxi for the first time and learning how lxml's parser wraps libxml2. I include a little bit of detail on libxml2 internals because it appears coupled to lxml and I don't anticipate any changes soon to the libxml2 code I mention.

Feedback is welcome.

…er.pxi

scoder

Thanks.

I believe that documentation/docstrings should not repeat code. (Some of your proposals really just state what the code does.) It should rather explain and motivate what's not obvious from the code, why it's done, why it's needed.

scoder · 2025-02-03T13:54:11Z

src/lxml/parser.pxi

@@ -573,6 +573,9 @@ cdef class _ParserContext(_ResolverContext):
        return context

    cdef void _initParserContext(self, xmlparser.xmlParserCtxt* c_ctxt) noexcept:
+        """
+        Connects the libxml2-level context to the lxml-level context.


Suggested change

Connects the libxml2-level context to the lxml-level context.

Connects the libxml2-level context to the lxml-level parser context.

scoder · 2025-02-03T13:58:06Z

src/lxml/parser.pxi

+        """
+        This method resets the parser context for a new parse. It also connects the
+        error log to the libxml-level ctxt by passing in _receiveParserError in as a
+        callback. Note: the HTML parser always has the libxml2 ctxt->sax not set to
+        null, so this always works. The ctxt->sax is set up in the libxml2 funciton
+        htmlInitParserCtxt in HTMLparser.c, and the lxml error log is always connected.
+        Then libxml2's SAX's serror is set to be the place errors are sent when schannel
+        is set to ctxt->sax->serror in xmlCtxtErrMemory in libxml2's parserInternals.c.
+        """


Documentation should not duplicate code, only explain it.

Suggested change

"""

This method resets the parser context for a new parse. It also connects the

error log to the libxml-level ctxt by passing in _receiveParserError in as a

callback. Note: the HTML parser always has the libxml2 ctxt->sax not set to

null, so this always works. The ctxt->sax is set up in the libxml2 funciton

htmlInitParserCtxt in HTMLparser.c, and the lxml error log is always connected.

Then libxml2's SAX's serror is set to be the place errors are sent when schannel

is set to ctxt->sax->serror in xmlCtxtErrMemory in libxml2's parserInternals.c.

"""

"""

This method resets the parser context for a new parse. It also connects the

error log to the libxml-level ctxt.

"""

scoder · 2025-02-03T13:59:05Z

src/lxml/parser.pxi

@@ -687,6 +702,15 @@ cdef xmlDoc* _handleParseResult(_ParserContext context,
                                xmlparser.xmlParserCtxt* c_ctxt,
                                xmlDoc* result, filename,
                                bint recover, bint free_doc) except NULL:
+    """
+    C-level xmlDoc* result is set to NULL if the parser cannot parse the document.


This is better explained with an in-place code comment than in a docstring.

Thanks for the feedback! The problem is that result = NULL appears many times in parser.pxi. result's being NULL is also checked in many different places using result is NULL and result is not NULL throughout parser.pxi. So, an in-place code comment would have to be duplicated >10 times. Do you agree?

… or appear at the top of the function. But it's still a code comment, not a docstring. docstrings are written for users of the function. Code comments are written for readers of the code.

scoder · 2025-02-03T14:05:44Z

src/lxml/parser.pxi

+    """
+    C-level xmlDoc* result is set to NULL if the parser cannot parse the document.
+    Keep in mind the libxml2-level wellFormed, which starts at 1, is set to 0 if the
+    parser may be able to parse the document, but there are errors that libxml2 has
+    to work through. lxml then uses this, along with some other logic, to determine
+    whether to set lxml-level well_formed to 0 and if that happens, set result to
+    NULL. Then, if result is NULL, raise a parse error. Some modifications to result
+    are made. Then the xmlDoc* result is returned.
+    """


Suggested change

"""

C-level xmlDoc* result is set to NULL if the parser cannot parse the document.

Keep in mind the libxml2-level wellFormed, which starts at 1, is set to 0 if the

parser may be able to parse the document, but there are errors that libxml2 has

to work through. lxml then uses this, along with some other logic, to determine

whether to set lxml-level well_formed to 0 and if that happens, set result to

NULL. Then, if result is NULL, raise a parse error. Some modifications to result

are made. Then the xmlDoc* result is returned.

"""

"""

Determine if we can consider the parsing successful.

Handles different error cases, e.g. with 'recover=True' or validation.

Frees a parsed document if it turns out not to be acceptable.

Raises an appropriate parse error.

"""

This is mostly correct. However, I think the sentence Handles different error cases, e.g. with 'recover=True' or validation. is too simple. For example the block

elif recover or (c_ctxt.wellFormed and c_ctxt.lastError.level < xmlerror.XML_ERR_ERROR): well_formed = 1

is not so much describing any particular kind of error as much as saying to carry over libxml2 errors unless in lxml level recovery mode or libxml2 lastError has a level at least as high as than what seems to be an error level named XML_ERR_ERROR. And, it's important to understand the underlying roles of the variables c_ctxt.wellFormed and well_formed because they're different but made of the same two English words. (I mean English words as opposed to variable names or keywords.)

The reason my docstring was originally long is because the code itself is very complex. What do you think?

So, if specifically the case in the code that you copied above is not clear, why not just add a comment to it?

I'm happy to add a comment to it rather then a docstring. But where does this leave us on content? Should I put what I had in the original PR, with docstrings replaced by code comments? Or what I mention above in my previous PR comment (not to be confused with code comments)? Or does it matter? I just want to be sure you're okay with whatever I come up with.

scoder · 2025-02-03T14:06:28Z

src/lxml/parser.pxi

+        """
+        This method creates a parser context if there isn't already one. If
+        creating a new parser context, this function configures it, creates a
+        libxml2-level parser context, connects the two, and makes some changes
+        to the libxml2 context. Then, it returns the parser context for the
+        parser.
+        """


Suggested change

"""

This method creates a parser context if there isn't already one. If

creating a new parser context, this function configures it, creates a

libxml2-level parser context, connects the two, and makes some changes

to the libxml2 context. Then, it returns the parser context for the

parser.

"""

scoder · 2025-02-03T14:07:58Z

src/lxml/parser.pxi

+        """
+        This method configures the lxml-level parser.
+        """


Suggested change

"""

This method configures the lxml-level parser.

"""

"""

Create and configure an appropriate lxml-level parser context.

"""

scoder · 2025-02-03T14:13:17Z

src/lxml/parser.pxi

+        """
+        This method calls into libxml to configure the libxml2-level parser context,
+        among other things.
+        """


Suggested change

"""

This method calls into libxml to configure the libxml2-level parser context,

among other things.

"""

"""

Create a libxml2-level parser context.

"""

The reason I wrote among other things is that the lines
self._registerHtmlErrorHandler(c_ctxt)
and
c_ctxt.sax.startDocument = _initSaxDocument
are important and not related to creating the parser context. This is especially true with the line about SAX, which does many things.

Well, there is a libxml2 function to create the context. If the creation was trivial, we wouldn't need this function here just to call it.

Suggested change

"""

This method calls into libxml to configure the libxml2-level parser context,

among other things.

"""

"""

Create and initialise a libxml2-level parser context.

"""

Better?

I would advocate getting rid of the docstring entirely and replace it with comments in the right places. We could avoid the words "among other things" by just not putting a comment above the lines with "other things" (for example c_ctxt.sax.startDocument = _initSaxDocument).

abepolk · 2025-02-06T14:12:15Z

Thanks for the feedback! I'm happy to make the changes you suggested. But first, I want to ask whether there is a better way to make lxml's parser.pxi more accessible to newcomers. Would a diagram, or a gloss of some sort be helpful? Or a list of common variables and what they represent? I'd like to make it so that people can read parser.pxi and quickly learn how it works. Thanks again!

scoder · 2025-02-09T13:25:52Z

Would a diagram, or a gloss of some sort be helpful? Or a list of common variables and what they represent?

Since you digged into it, you're probably in the best position to explain what information you would have liked to have. Then we could think about how best to present it.

abepolk · 2025-02-10T01:15:11Z

That makes sense! I'll think more about that. In the meanwhile, I'll accept your suggestions soon so we can merge the docstrings. I'm also going to send two other more minor PRs. Besides the PRs, I'm in the final stages of writing an article on Medium explaining some other work I've done on researching lxml and libxml2 errors.

abepolk · 2025-02-15T19:53:39Z

I was thinking that instead of these docstrings, I could instead write an unofficial gloss or "guide" to reading the code in a separate article on Medium. My sense is that there is a lot of complexity in lxml and libxml2 and coupling between the two. An unofficial gloss or guide might be able to explain the complexity better. Any thoughts on that?

scoder · 2025-02-15T20:00:49Z

write an unofficial gloss or "guide" to reading the code in a separate article

That would be a separate article and many people won't expect such an article to exist (and be worth searching for) when starting to read the code. Comments in the code would certainly be more helpful.

I write "comments" because code concepts are better explained in comments than in docstrings. docstrings are for users of the library. Comments are for readers of the source code.

You can also add to this document:
https://lxml.de/lxml-source-howto.html
It's at least closer to the code than a "random article" on Medium.

(PS: As you probably notice from that doc page, external documentation tends to get outdated pretty reliably over time.)

abepolk · 2025-02-18T00:28:08Z

That makes sense. I left a few comments on the specific suggested changes in the PR. Overall, the plan of making this PR about code comments rather than docstrings makes sense to me. It could also make sense to add to the documentation on lxml.de. Does this mean the content I originally wrote might be worth keeping? For example, the parts about how libxml2 internals affect lxml?

scoder · 2025-02-18T05:08:30Z

Does this mean the content I originally wrote might be worth keeping?

Since it seemed to help you, it probably is. If you split it into code comments in place, we'll see whether they are really helpful or redundant.

… docstrings and some comments

abepolk · 2025-02-21T21:26:20Z

I converted a number of the docstrings to comments. I also reworded and removed a bunch. I tried to take your feedback into consideration. Let me know what you think! And, no problem if this PR doesn't work out—I will keep my notes for future times I come to read parser.pxi.

src/lxml/parser.pxi

@@ -687,6 +702,15 @@ cdef xmlDoc* _handleParseResult(_ParserContext context,
                                xmlparser.xmlParserCtxt* c_ctxt,


src/lxml/parser.pxi

scoder · 2025-04-22T12:04:15Z

Thanks

abepolk · 2025-04-24T19:20:59Z

Glad you merged this! Thanks!

abepolk added 2 commits January 29, 2025 21:38

Add docstrings relating to functions involving parser context to pars…

58f9827

…er.pxi

Fix indent

5c8073c

scoder reviewed Feb 3, 2025

View reviewed changes

Change some docstrings to comments and then reword and/or remove some…

eebdbc2

… docstrings and some comments

SAIVEERENDER24 reviewed Apr 17, 2025

View reviewed changes

src/lxml/parser.pxi

@@ -687,6 +702,15 @@ cdef xmlDoc* _handleParseResult(_ParserContext context,

xmlparser.xmlParserCtxt* c_ctxt,

This comment was marked as spam.

Sign in to view

scoder reviewed Apr 22, 2025

View reviewed changes

src/lxml/parser.pxi Outdated Show resolved Hide resolved

Deflate comment.

3851943

scoder merged commit c7828c9 into lxml:master Apr 22, 2025
0 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docstrings relating to functions involving parser context to parser.pxi #449

Add docstrings relating to functions involving parser context to parser.pxi #449

abepolk commented Jan 29, 2025

scoder left a comment

scoder Feb 3, 2025

scoder Feb 3, 2025

scoder Feb 3, 2025

abepolk Feb 12, 2025

scoder Feb 15, 2025

scoder Feb 3, 2025

abepolk Feb 12, 2025 •

edited

Loading

scoder Feb 15, 2025

abepolk Feb 18, 2025

scoder Feb 3, 2025

scoder Feb 3, 2025

scoder Feb 3, 2025

abepolk Feb 12, 2025

scoder Feb 15, 2025 •

edited

Loading

abepolk Feb 18, 2025

abepolk commented Feb 6, 2025

scoder commented Feb 9, 2025

abepolk commented Feb 10, 2025

abepolk commented Feb 15, 2025

scoder commented Feb 15, 2025 •

edited

Loading

abepolk commented Feb 18, 2025 •

edited

Loading

scoder commented Feb 18, 2025

abepolk commented Feb 21, 2025

This comment was marked as spam.

scoder commented Apr 22, 2025

abepolk commented Apr 24, 2025

	Connects the libxml2-level context to the lxml-level context.
	Connects the libxml2-level context to the lxml-level parser context.

		@@ -687,6 +702,15 @@ cdef xmlDoc* _handleParseResult(_ParserContext context,
		xmlparser.xmlParserCtxt* c_ctxt,

Add docstrings relating to functions involving parser context to parser.pxi #449

Add docstrings relating to functions involving parser context to parser.pxi #449

Conversation

abepolk commented Jan 29, 2025

scoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abepolk Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scoder Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abepolk commented Feb 6, 2025

scoder commented Feb 9, 2025

abepolk commented Feb 10, 2025

abepolk commented Feb 15, 2025

scoder commented Feb 15, 2025 • edited Loading

abepolk commented Feb 18, 2025 • edited Loading

scoder commented Feb 18, 2025

abepolk commented Feb 21, 2025

This comment was marked as spam.

scoder commented Apr 22, 2025

abepolk commented Apr 24, 2025

abepolk Feb 12, 2025 •

edited

Loading

scoder Feb 15, 2025 •

edited

Loading

scoder commented Feb 15, 2025 •

edited

Loading

abepolk commented Feb 18, 2025 •

edited

Loading