gh-128508: Add some docstrings to xml.dom.minidom #128477

srinivasreddy · 2025-01-04T10:34:38Z

Initial version extracted from PR #24152, which also deleted and modified minidom tests.

Co-authored-by: Karl Dubost karl+github@la-grange.net

Issue: Add some docstrings to xml.dom.minidom #128508

Lib/xml/dom/minidom.py

terryjreedy

I have not worked on docstrings, especially not for anything other that IDLE, for years. The best reference I know is the Multiline String section of https://peps.python.org/pep-0257/, though the thoroughness described there may be rare. Still, these need more work. The xml.dom and xml.dom.minidom docs should be a basis for corresponding docstrings. Usefully summarizing behavior can be hard.

I think a blurb should be added, but this can wait.

Lib/xml/dom/minidom.py

terryjreedy · 2025-01-05T02:02:19Z

Lib/xml/dom/minidom.py

    """
-    Clone a node and give it the new owner document.
+    Returns a copy of node.


Remove \n after """. Add after this line. But this revision seems wrong as it omits mention of newOwnerDocument. I would have to read code to suggest improvement.

terryjreedy · 2025-01-05T02:05:24Z

Lib/xml/dom/minidom.py

@@ -204,6 +219,7 @@ def normalize(self):
        self.childNodes[:] = L

    def cloneNode(self, deep):
+        """The Node.cloneNode() method returns a duplicate of the node."""


Should be "Return a duplicate ...", but needs to explain 'deep' (I suspect it is a boolean.) Docstrings are for quick reminders when coding, and 'deep' is not obvious.

terryjreedy · 2025-01-05T02:05:45Z

Lib/xml/dom/minidom.py

@@ -1341,6 +1357,7 @@ def _get_internalSubset(self):
        return self.internalSubset

    def cloneNode(self, deep):
+        """The Node.cloneNode() method returns a duplicate of the node."""


See other cloneNode comment.

terryjreedy · 2025-01-05T02:07:29Z

Lib/xml/dom/minidom.py

@@ -129,6 +138,7 @@ def appendChild(self, node):
        return node

    def replaceChild(self, newChild, oldChild):
+        """Replace an existing node with a new node."""


Suggested change

"""Replace an existing node with a new node."""

"""Replace child node *oldChild* with *newChild*."""

bedevere-app · 2025-01-05T02:39:37Z

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 phrase I 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.

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>

…ide C comments (python#128464)

…o()` (python#128337) These methods combine `_delete()` and `copy()`, but `_delete()` isn't part of the public interface, and it's unlikely to be added until the pathlib ABCs are made official, or perhaps even later.

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

…er test (python#128338) In the tests for `pathlib.Path.walk()`, avoid using the path class under test (`self.cls`) in test setup. Instead we use `os` functions in `test_pathlib`, and direct manipulation of `DummyPath` internal data in `test_pathlib_abc`.

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

…127564)

…on#128502)

pythongh-128455) Add `BOLT_COMMON_FLAGS` with `-update-debug-sections` Co-authored-by: Gregory Szorc <gregory.szorc@gmail.com>

…he atomic operation (pythongh-128196)

Lib/xml/dom/minidom.py

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

merwok · 2025-01-09T19:09:27Z

Lib/xml/dom/minidom.py

@@ -32,6 +32,7 @@


 class Node(xml.dom.Node):
+    """Define properties accessible on a DOM node."""


I think it’s strange to have a class docstring start with a verb; it makes sense for a method (that does something), but a class usually represents a thing (noun), it does not do something.

Addressed in b97d812

merwok · 2025-01-09T19:11:12Z

Lib/xml/dom/minidom.py

+        https://dom.spec.whatwg.org/#dom-node-insertbefore
+        The insertBefore(node, child) method, when invoked,
+        must return the result of pre-inserting node into
+        this before child.


I think the docstring should document what the method does, not quote parts of a spec.

I would suggest simpler wording, but I don’t understand the existing one («the result of pre-inserting node into this before child» ?!)

Addressed in b97d812

merwok · 2025-01-09T19:11:28Z

Lib/xml/dom/minidom.py

+        The insertBefore(node, child) method, when invoked,
+        must return the result of pre-inserting node into
+        this before child.
+        See also https://dom.spec.whatwg.org/#concept-node-pre-insert


I would have links like this in the reST docs, not docstrings.

Addressed in b97d812

Update and Add docstrings for functions and methods in minidom.py module

21bb374

bedevere-app bot added the awaiting review label Jan 4, 2025

srinivasreddy commented Jan 4, 2025

View reviewed changes

Lib/xml/dom/minidom.py Outdated Show resolved Hide resolved

srinivasreddy changed the title ~~Update and Add docstrings for functions and methods in minidom.py module~~ gh-63882: Update and Add docstrings for functions and methods in minidom.py module Jan 4, 2025

bedevere-app bot mentioned this pull request Jan 4, 2025

test_minidom has many empty tests #63882

Open

terryjreedy changed the title ~~gh-63882: Update and Add docstrings for functions and methods in minidom.py module~~ gh-128508: Update and Add docstrings for functions and methods in minidom.py module Jan 5, 2025

bedevere-app bot mentioned this pull request Jan 5, 2025

Add some docstrings to xml.dom.minidom #128508

Open

terryjreedy added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jan 5, 2025

terryjreedy changed the title ~~gh-128508: Update and Add docstrings for functions and methods in minidom.py module~~ gh-128508: Add some docstrings to xml.dom.minidom Jan 5, 2025

terryjreedy requested changes Jan 5, 2025

View reviewed changes

bedevere-app bot removed the awaiting review label Jan 5, 2025

bedevere-app bot added the awaiting changes label Jan 5, 2025

terryjreedy mentioned this pull request Jan 5, 2025

gh-63882: Adds tests for xml.dom.minidom #24152

Closed

srinivasreddy and others added 15 commits January 6, 2025 11:34

Update Lib/xml/dom/minidom.py

f16761b

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>

Update Lib/xml/dom/minidom.py

eb0208b

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>

Update Lib/xml/dom/minidom.py

f4f6334

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>

Add check-readthedocs pre-commit hook (python#128453)

70617ea

pythongh-128152: Argument Clinic: ignore pre-processor directives ins…

90b344f

…ide C comments (python#128464)

Docs: mark up json.dump() using parameter list (python#128482)

5f7ba82

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

pythongh-126719: Clarify math.fmod docs (python#127741)

e6a8d38

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Docs: amend json.dump() post pythongh-128482 (python#128489)

1592145

pythongh-127954: Document PyObject_DelItemString (python#127986)

1a8ee69

pythongh-127553: Remove outdated TODO comment in _pydatetime (python#…

61c3e8a

…127564)

pythongh-115765: Document and enforce Autoconf 2.72 requirement (pyth…

69f8e4b

…on#128502)

pythongh-128437: Add BOLT_COMMON_FLAGS with -update-debug-sections (

7f767cd

pythongh-128455) Add `BOLT_COMMON_FLAGS` with `-update-debug-sections` Co-authored-by: Gregory Szorc <gregory.szorc@gmail.com>

pythongh-128137: Update PyASCIIObject to handle interned field with t…

2f8b072

…he atomic operation (pythongh-128196)

hugovk removed request for abalkin, barneygale, hugovk, pganssle, corona10, erlend-aasland, ezio-melotti and AlexWaygood January 6, 2025 07:37

erlend-aasland requested changes Jan 6, 2025

View reviewed changes

Lib/xml/dom/minidom.py Outdated Show resolved Hide resolved

Lib/xml/dom/minidom.py Outdated Show resolved Hide resolved

erlend-aasland added the skip news label Jan 6, 2025

Convert doc string to imperative mode

a4840f3

erlend-aasland reviewed Jan 6, 2025

View reviewed changes

Lib/xml/dom/minidom.py Outdated Show resolved Hide resolved

srinivasreddy added 6 commits January 6, 2025 15:06

Update docstring as recommended by argument clinic

38be045

Update docstring as recommended by argument clinic

cca0fda

Undo removing comment

8df6795

Update docstrings as it was done in argument clinic

727af86

Add space back

2da171f

Update doc strings

ecb4a54

erlend-aasland reviewed Jan 6, 2025

View reviewed changes

Lib/xml/dom/minidom.py Outdated Show resolved Hide resolved

Lib/xml/dom/minidom.py Outdated Show resolved Hide resolved

srinivasreddy and others added 2 commits January 6, 2025 18:13

Update Lib/xml/dom/minidom.py

8b63730

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update Lib/xml/dom/minidom.py

49a12e6

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

srinivasreddy requested a review from erlend-aasland January 7, 2025 09:45

merwok reviewed Jan 9, 2025

View reviewed changes

srinivasreddy added 2 commits January 16, 2025 18:17

Merge branch 'main' into pythongh-63882-doc_strings

298a20f

Address review comments

b97d812

srinivasreddy requested a review from merwok January 16, 2025 13:03

Update docstrings

19d245c

srinivasreddy mentioned this pull request Jan 17, 2025

gh-63882: Comment out empty test_minidom tests #128948

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-128508: Add some docstrings to xml.dom.minidom #128477

gh-128508: Add some docstrings to xml.dom.minidom #128477

srinivasreddy commented Jan 4, 2025 •

edited by terryjreedy

Loading

terryjreedy left a comment •

edited

Loading

terryjreedy Jan 5, 2025

terryjreedy Jan 5, 2025

terryjreedy Jan 5, 2025

terryjreedy Jan 5, 2025

bedevere-app bot commented Jan 5, 2025

merwok Jan 9, 2025

srinivasreddy Jan 16, 2025

merwok Jan 9, 2025

srinivasreddy Jan 16, 2025

merwok Jan 9, 2025

srinivasreddy Jan 16, 2025

	"""Replace an existing node with a new node."""
	"""Replace child node oldChild with newChild."""

		@@ -32,6 +32,7 @@


		class Node(xml.dom.Node):
		"""Define properties accessible on a DOM node."""

gh-128508: Add some docstrings to xml.dom.minidom #128477

Are you sure you want to change the base?

gh-128508: Add some docstrings to xml.dom.minidom #128477

Conversation

srinivasreddy commented Jan 4, 2025 • edited by terryjreedy Loading

terryjreedy left a comment • 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

bedevere-app bot commented Jan 5, 2025

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

srinivasreddy commented Jan 4, 2025 •

edited by terryjreedy

Loading

terryjreedy left a comment •

edited

Loading