GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-128508: Add some docstrings to xml.dom.minidom by srinivasreddy · Pull Request #128477 · python/cpython

Conversation

@srinivasreddy

@srinivasreddy srinivasreddy commented Jan 4, 2025 •
edited by terryjreedy
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Co-authored-by: Karl Dubost [email protected]

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
karlcow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@srinivasreddy
Update and Add docstrings for functions and methods in minidom.py module
21bb374
@bedevere-app bedevere-app Bot added the awaiting review label Jan 4, 2025
srinivasreddy
srinivasreddy commented Jan 4, 2025
View reviewed changes
Comment thread Lib/xml/dom/minidom.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@srinivasreddy 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 bedevere-app Bot mentioned this pull request Jan 4, 2025
test_minidom has many empty tests #63882
Closed
@terryjreedy 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 bedevere-app Bot mentioned this pull request Jan 5, 2025
Add some docstrings to xml.dom.minidom #128508
Open
@terryjreedy terryjreedy added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Jan 5, 2025
@terryjreedy 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
terryjreedy requested changes Jan 5, 2025
View reviewed changes

@terryjreedy terryjreedy left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/xml/dom/minidom.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/xml/dom/minidom.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/xml/dom/minidom.py Outdated
Comment on lines +1923 to +1924
"""
Clone a node and give it the new owner document.
Returns a copy of node.

@terryjreedy terryjreedy Jan 5, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/xml/dom/minidom.py Outdated
self.childNodes[:] = L

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

@terryjreedy terryjreedy Jan 5, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/xml/dom/minidom.py Outdated
return self.internalSubset

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

@terryjreedy terryjreedy Jan 5, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

See other cloneNode comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/xml/dom/minidom.py Outdated
return node

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

@terryjreedy terryjreedy Jan 5, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot removed the awaiting review label Jan 5, 2025
@bedevere-app

bedevere-app Bot commented Jan 5, 2025

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting changes label Jan 5, 2025
@terryjreedy 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
@srinivasreddy @terryjreedy
Update Lib/xml/dom/minidom.py
f16761b 
Co-authored-by: Terry Jan Reedy <[email protected]>
@srinivasreddy @terryjreedy
Update Lib/xml/dom/minidom.py
eb0208b 
Co-authored-by: Terry Jan Reedy <[email protected]>
@srinivasreddy @terryjreedy
Update Lib/xml/dom/minidom.py
f4f6334 
Co-authored-by: Terry Jan Reedy <[email protected]>
@sobolevn @srinivasreddy
Add check-readthedocs pre-commit hook (python#128453)
70617ea
@erlend-aasland @srinivasreddy
pythongh-128152: Argument Clinic: ignore pre-processor directives ins…
90b344f 
…ide C comments (python#128464)
@barneygale @srinivasreddy
pythonGH-127381: pathlib ABCs: remove PathBase.move() and `move_int…
34c317a 
…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.
@erlend-aasland @hugovk @srinivasreddy
Docs: mark up json.dump() using parameter list (python#128482)
5f7ba82 
Co-authored-by: Hugo van Kemenade <[email protected]>
@barneygale @srinivasreddy
pathlib tests: create walk() test hierarchy without using class und…
23d11be 
…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`.
@StanFromIreland @picnixz @srinivasreddy
pythongh-126719: Clarify math.fmod docs (python#127741)
e6a8d38 
Co-authored-by: Bénédikt Tran <[email protected]>
@erlend-aasland @srinivasreddy
Docs: amend json.dump() post pythongh-128482 (python#128489)
1592145
@rruuaanng @srinivasreddy
pythongh-127954: Document PyObject_DelItemString (python#127986)
1a8ee69
@bombs-kim @srinivasreddy
pythongh-127553: Remove outdated TODO comment in _pydatetime (python#…
61c3e8a 
…127564)
@erlend-aasland @srinivasreddy
pythongh-115765: Document and enforce Autoconf 2.72 requirement (pyth…
69f8e4b 
…on#128502)
@zanieb @indygreg @srinivasreddy
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 <[email protected]>
@corona10 @srinivasreddy
pythongh-128137: Update PyASCIIObject to handle interned field with t…
2f8b072 
…he atomic operation (pythongh-128196)
erlend-aasland
erlend-aasland reviewed Jan 6, 2025
View reviewed changes
Comment thread Lib/xml/dom/minidom.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
srinivasreddy added 6 commits January 6, 2025 15:06
@srinivasreddy
Update docstring as recommended by argument clinic
38be045
@srinivasreddy
Update docstring as recommended by argument clinic
cca0fda
@srinivasreddy
Undo removing comment
8df6795
@srinivasreddy
Update docstrings as it was done in argument clinic
727af86
@srinivasreddy
Add space back
2da171f
@srinivasreddy
Update doc strings
ecb4a54
erlend-aasland
erlend-aasland reviewed Jan 6, 2025
View reviewed changes
Comment thread Lib/xml/dom/minidom.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/xml/dom/minidom.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
srinivasreddy and others added 2 commits January 6, 2025 18:13
@srinivasreddy @erlend-aasland
Update Lib/xml/dom/minidom.py
8b63730 
Co-authored-by: Erlend E. Aasland <[email protected]>
@srinivasreddy @erlend-aasland
Update Lib/xml/dom/minidom.py
49a12e6 
Co-authored-by: Erlend E. Aasland <[email protected]>
@srinivasreddy
srinivasreddy requested a review from erlend-aasland January 7, 2025 09:45
merwok
merwok reviewed Jan 9, 2025
View reviewed changes
Comment thread Lib/xml/dom/minidom.py Outdated


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

@merwok merwok Jan 9, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@srinivasreddy srinivasreddy Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Addressed in b97d812

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
merwok
merwok reviewed Jan 9, 2025
View reviewed changes
Comment thread Lib/xml/dom/minidom.py Outdated
Comment on lines +87 to +90
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.

@merwok merwok Jan 9, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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» ?!)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@srinivasreddy srinivasreddy Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Addressed in b97d812

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
merwok
merwok reviewed Jan 9, 2025
View reviewed changes
Comment thread Lib/xml/dom/minidom.py Outdated
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

@merwok merwok Jan 9, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@srinivasreddy srinivasreddy Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Addressed in b97d812

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
srinivasreddy added 2 commits January 16, 2025 18:17
@srinivasreddy
Merge branch 'main' into pythongh-63882-doc_strings
298a20f
@srinivasreddy
Address review comments
b97d812
@srinivasreddy
srinivasreddy requested a review from merwok January 16, 2025 13:03
@srinivasreddy
Update docstrings
19d245c
@srinivasreddy srinivasreddy mentioned this pull request Jan 17, 2025
gh-63882: Comment out empty test_minidom tests #128948
Closed
@tomasr8 tomasr8 removed the needs backport to 3.12 only security fixes label Apr 10, 2025
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.14 bugs and security fixes label May 8, 2025
erlend-aasland
erlend-aasland reviewed May 12, 2025
View reviewed changes
Comment thread Lib/xml/dom/minidom.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
srinivasreddy and others added 2 commits May 13, 2025 09:45
@srinivasreddy @erlend-aasland
Update Lib/xml/dom/minidom.py
7b1666d 
Co-authored-by: Erlend E. Aasland <[email protected]>
@srinivasreddy
Merge branch 'main' into pythongh-63882-doc_strings
c68cb4d
@github-actions

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 20, 2026
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.15 bugs and security fixes label May 30, 2026
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label May 31, 2026
@serhiy-storchaka serhiy-storchaka mentioned this pull request Aug 30, 2026
xml.dom.minidom.Document class is not documented #57952
Closed
@serhiy-storchaka

serhiy-storchaka commented Aug 30, 2026

Copy link
Copy Markdown
Member

The formatting concerns from the earlier reviews look addressed. Some remarks on the content:

  • toxml() returns bytes, not a string, if encoding is given. toprettyxml(), which does the actual work and has the interesting parameters, has no docstring.
  • "exclusive Text nodes" in normalize() is not DOM terminology. It is also worth mentioning that the method is recursive.
  • DocumentType.cloneNode() returns None if the node has an owner document, so the docstring copied from Node.cloneNode() does not describe it.
  • The change to _clone_node() is unrelated: it is private and already had a docstring, and the note "Called by Node.cloneNode and Document.importNode" was more useful than the parameter list replacing it.

Also, this only adds a docstring for the Node class; Document, Element, Attr and others still have none, which is what gh-57952 was left open for.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland requested changes
@terryjreedy terryjreedy Awaiting requested review from terryjreedy
@merwok merwok Awaiting requested review from merwok

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 bugs and security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

15 participants

@srinivasreddy @serhiy-storchaka @merwok @erlend-aasland @terryjreedy @tomasr8 @sobolevn @barneygale @StanFromIreland @rruuaanng @bombs-kim @zanieb @corona10 @Damien-Chen @RanKKI
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.