Skip to content

gh-126273: added clinic docstrings to elementtree - #126274

Closed
pan324 wants to merge 6 commits into
python:mainfrom
pan324:etreedocs
Closed

gh-126273: added clinic docstrings to elementtree#126274
pan324 wants to merge 6 commits into
python:mainfrom
pan324:etreedocs

Conversation

@pan324

@pan324pan324 commented Nov 1, 2024

Copy link
Copy Markdown
Contributor

@skirpichev

Copy link
Copy Markdown
Member

clinic/* files are autogenerated by Tools/clinic/clinic.py. You should modify instead clinic input - Modules/_elementtree.c in your case, I guess.

See also: https://devguide.python.org/development-tools/clinic/

@pan324

Copy link
Copy Markdown
ContributorAuthor

Thanks, will do that now.

@skirpichev
skirpichev self-requested a review November 1, 2024 13:08
…6273.t_qOoH.rst
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Comment on lines +1259 to +1260
*path* is a string having either an element tag or an XPath,
*namespaces* is an optional mapping from namespace prefix to full name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider using per-parameter docstrings here (see the Argument Clinic Howto for inspiration).

Consider this also for the rest of the PR.

*path* is a string having either an element tag or an XPath,
*namespaces* is an optional mapping from namespace prefix to full name.

Returns list containing all matching elements in document order.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please consistently use the imperative mood for reference prose. Ditto for the rest of the PR.

Suggested change
Returnslistcontainingallmatchingelementsindocumentorder.
Returnalistcontainingallmatchingelementsindocumentorder.

@skirpichev

This comment was marked as resolved.

@ZeroIntensity

This comment was marked as resolved.

@skirpichev

This comment was marked as resolved.

@erlend-aaslanderlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Normally we try to align the docstrings with the reference text in the documentation. In some places, this is already done (the added docstring is similar or identical to the reference docs), but in some places it differs significantly. If the reason for the deviation is that the docs are inaccurate, we should IMO fix them (first), and then align the added docstrings to the amended docs. Moreover, my previous round of review remarks have not been addressed yet.

@bedevere-app

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.

@skirpichev
skirpichev removed their request for review November 14, 2024 06:43
@python-cla-bot

Copy link
Copy Markdown

The following commit authors need to sign the Contributor License Agreement:

CLA signed

@github-actions

Copy link
Copy Markdown

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

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Apr 19, 2026
@StanFromIreland

Copy link
Copy Markdown
Member

Closing as the CLA is unsigned, there are conflicts and requested changes have not been made. Please create a new PR if you wish to pick this up again.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changesstaleStale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@pan324@skirpichev@ZeroIntensity@StanFromIreland@erlend-aasland