Movatterモバイル変換

srinivasreddy commented

Lib/xml/dom/minidom.py OutdatedShow resolvedHide 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

bedevere-appbot mentioned this pull request

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

bedevere-appbot mentioned this pull request

Add some docstrings to xml.dom.minidom#128508

Open

terryjreedy added needs backport to 3.12

only security fixes

needs backport to 3.13bugs and security fixes labels

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

terryjreedy requested changes

Copy link

Member

terryjreedy left a comment•
edited
Loading

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 ofhttps://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.pyShow resolvedHide resolved

Lib/xml/dom/minidom.py OutdatedShow resolvedHide resolved

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.

Copy link

Member

terryjreedyJan 5, 2025

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.

Lib/xml/dom/minidom.py Outdated

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

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

Copy link

Member

terryjreedyJan 5, 2025

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.

Lib/xml/dom/minidom.py Outdated

		@@ -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."""

Copy link

Member

terryjreedyJan 5, 2025

Choose a reason for hiding this comment

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

See other cloneNode comment.

Lib/xml/dom/minidom.py Outdated

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

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

Copy link

Member

terryjreedyJan 5, 2025

Choose a reason for hiding this comment

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

Suggested change

	"""Replacean existingnode witha new node."""
	"""ReplacechildnodeoldChildwithnewChild."""

bedevere-appbot removed the awaiting review label

Copy link

bedevere-appbot commentedJan 5, 2025

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 phraseI 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.

bedevere-appbot added the awaiting changes label

terryjreedy mentioned this pull request

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

Closed

srinivasreddyand others added15 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>

Addcheck-readthedocspre-commit hook (python#128453)

70617ea

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

90b344f

…ide C comments (python#128464)

pythonGH-127381: pathlib ABCs: removePathBase.move()and `move_int…

34c317a

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

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

5f7ba82

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

pathlib tests: createwalk() test hierarchy without using class und…

23d11be

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

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() postpythongh-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: AddBOLT_COMMON_FLAGSwith-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 forerlend-aasland,ezio-melotti andAlexWaygood

January 6, 2025 07:37

erlend-aasland requested changes

Lib/xml/dom/minidom.py OutdatedShow resolvedHide resolved

erlend-aasland added the skip news label

Convert doc string to imperative mode

a4840f3

erlend-aasland reviewed

Lib/xml/dom/minidom.py OutdatedShow resolvedHide resolved

srinivasreddy added6 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

Lib/xml/dom/minidom.py OutdatedShow resolvedHide resolved

srinivasreddyand others added2 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 fromerlend-aasland

January 7, 2025 09:45

merwok reviewed

Jan 9, 2025

Lib/xml/dom/minidom.py Outdated

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


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

Copy link

Member

merwokJan 9, 2025

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.

Copy link

ContributorAuthor

srinivasreddyJan 16, 2025

Choose a reason for hiding this comment

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

Addressed inb97d812

merwok reviewed

Jan 9, 2025

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.

Copy link

Member

merwokJan 9, 2025

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

Copy link

ContributorAuthor

srinivasreddyJan 16, 2025

Choose a reason for hiding this comment

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

Addressed inb97d812

merwok reviewed

Jan 9, 2025

gh-63882: Comment out emptytest_minidom tests#128948

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

Copy link

Member

merwokJan 9, 2025

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.

Copy link

ContributorAuthor

srinivasreddyJan 16, 2025

Choose a reason for hiding this comment

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

Addressed inb97d812

srinivasreddy added2 commits

January 16, 2025 18:17

Merge branch 'main' intopythongh-63882-doc_strings

298a20f

Address review comments

b97d812

srinivasreddy requested a review frommerwok

January 16, 2025 13:03

Update docstrings

19d245c

srinivasreddy mentioned this pull request

Jan 17, 2025

Open

tomasr8 removed the needs backport to 3.12only security fixes label

Apr 10, 2025

serhiy-storchaka added the needs backport to 3.14bugs and security fixes label

May 8, 2025

erlend-aasland reviewed

May 12, 2025