Skip to content

gh-90149: make pkutil path arg less ambiguous #119252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

gh-90149: make pkutil path arg less ambiguous #119252

wants to merge 1 commit into from
+13 −10

Conversation

drts01
Copy link
Contributor

@drts01 drts01 commented May 20, 2024 •
edited by bedevere-app bot
Loading

@drts01 drts01 requested a review from ericsnowcurrently as a code owner May 20, 2024 19:29
@ghost
Copy link

ghost commented May 20, 2024 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app bedevere-app bot mentioned this pull request May 20, 2024
Open
@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from 3b52969 to c976e60 Compare May 20, 2024 19:30
@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from c976e60 to b608b25 Compare May 20, 2024 19:33
@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from b608b25 to fca04c5 Compare May 20, 2024 19:33
@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from fca04c5 to 2f464e3 Compare May 20, 2024 20:07
@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from 2f464e3 to b9e7513 Compare May 20, 2024 20:07
@bedevere-app
Copy link

bedevere-app bot commented May 20, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01
Copy link
Contributor Author

drts01 commented May 20, 2024

Is "skip news" appropriate for this PR?

savannahostrowski
savannahostrowski reviewed May 21, 2024
View reviewed changes
Copy link
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me! I had one comment, but it's a style nit.

Doc/library/pkgutil.rst Outdated
@@ -127,7 +127,8 @@ support.
Yields :class:`ModuleInfo` for all submodules on *path*, or, if
*path* is ``None``, all top-level modules on :data:`sys.path`.

*path* should be either ``None`` or a list of paths to look for modules in.
*path* should be either ``None`` or a list of paths as strings to search
Copy link
Member

Choose a reason for hiding this comment

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

An alternative here might be to specify the type in parens, which might be easier to read:
"...a list of paths (List[str])..."
"...a list of paths (where each path is str)..."

...or similar. Not married to it, but I thought it might be worth considering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems reasonable. TBH, I was struggling to figure out the phrasing.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from b9e7513 to 1722a37 Compare May 21, 2024 15:19
@drts01 drts01 requested review from hugovk and AlexWaygood as code owners May 21, 2024 15:19
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from 1722a37 to 47faf92 Compare May 21, 2024 15:20
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from 47faf92 to ba84dcf Compare May 21, 2024 17:57
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from ba84dcf to 15f2f9b Compare May 21, 2024 18:35
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from 15f2f9b to 6a8f6a2 Compare May 21, 2024 22:32
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from 6a8f6a2 to d0d8054 Compare May 21, 2024 22:32
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from d0d8054 to 72f7a72 Compare May 21, 2024 22:33
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@savannahostrowski
Copy link
Member

I believe that skip news is appropriate here but we'll need to get a maintainer to add the label!

@drts01 drts01 force-pushed the pkutil_docstrings_90149 branch from 72f7a72 to c686707 Compare May 27, 2024 23:42
@donBarbos
Copy link
Contributor

@drts01 please do not force-push; it makes reviewing harder. Moreover, all commits are squashed upon merge anyway, so there's no need for the PR/branch to be cluttered with amendment commits. Source: devguide.

@savannahostrowski since the last change you became core member and it seems you can add "Skip" label

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

@savannahostrowski savannahostrowski savannahostrowski left review comments

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees
No one assigned
Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve ambiguous docstrings in pkgutil
3 participants
@drts01 @savannahostrowski @donBarbos