cpython icon indicating copy to clipboard operation
cpython copied to clipboard

Improve ambiguous docstrings in pkgutil

Open f800bbf7-261c-45d0-b680-bcbbf2e16193 opened this issue 4 years ago • 9 comments

BPO 45991
Nosy @barneygale, @slateny

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-12-05.18:56:47.235>
labels = ['easy', '3.9', '3.10', '3.11', 'type-feature', 'docs']
title = 'Improve ambiguous docstrings in pkgutil'
updated_at = <Date 2022-03-01.01:22:11.805>
user = 'https://bugs.python.org/khock'

bugs.python.org fields:

activity = <Date 2022-03-01.01:22:11.805>
actor = 'slateny'
assignee = 'docs@python'
closed = False
closed_date = None
closer = None
components = ['Documentation']
creation = <Date 2021-12-05.18:56:47.235>
creator = 'khock'
dependencies = []
files = []
hgrepos = []
issue_num = 45991
keywords = ['easy']
message_count = 7.0
messages = ['407727', '413883', '413933', '413957', '414076', '414212', '414230']
nosy_count = 4.0
nosy_names = ['docs@python', 'barneygale', 'khock', 'slateny']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue45991'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

  • gh-119252

# Issue

If you search for "list of paths" in https://github.com/KevinHock/cpython/blob/main/Lib/pkgutil.py

A lot of people mistake this as PosixPath. You can see an example here: https://github.com/duo-labs/parliament/pull/207 that references other OSS repositories.

# Solution

We can

  • Change the wording. e.g. "list of strings of the paths" or some variation of that.

and perhaps additionally

  • Throw a ValueError similar to: https://github.com/python/cpython/blob/628abe4463ed40cd54ca952a2b4cc2d6e74073f7/Lib/pkgutil.py#L122

Could you expand a bit on why 'list of paths' in pkgutil is understood by default to be 'list of PosixPath paths'? I would interpret it by default to be string paths if I saw it somewhere without context

slateny avatar Feb 24 '22 06:02 slateny

At best it is ambiguous, with the class being confused with Str being called Path. Looking up "AttributeError: 'PosixPath' object has no attribute 'startswith'" gives a lot of results for similar issues, so I think the wording could be improved.

While it is ambiguous, when there's a path parameter I would default it to string unless otherwise specified. Maybe instead a note could be put in the Pathlib doc noting functions that accept path arguments might not accept Path objects?

slateny avatar Feb 25 '22 03:02 slateny

Should pkgutil call os.fspath() in this case?

Maybe instead a note could be put in the Pathlib doc noting functions that accept path arguments might not accept Path objects?

My concern with that is that someone using pkgutil wouldn't see it. However, I can see the argument that fixing the 'source' is better than each use. I'm not sure how wide-spread these kind of issues are to weigh in on how many 'uses' there are. If that makes sense.

Should pkgutil call os.fspath() in this case?

I really like that idea. (I haven't contributed to CPython before, so I'll let someone else weigh in on if that is standard practice.)

I feel like there might be some backwards compatibility issues if pkgutil wraps it like that, but similarly I'm not at all familiar with how common the package is used and whether it'd be fine to make that change, so I'll also differ judgement here.

slateny avatar Mar 01 '22 01:03 slateny

Shouldn't this be closed? If not, I'd like to work on it.

safwansamsudeen avatar Jun 22 '23 12:06 safwansamsudeen

Shouldn't this be closed? If not, I'd like to work on it.

@safwansamsudeen I think you can work on it, just adding a note to the doc string saying that the path needs to be an str and/or adding a type check should be enough instead of converting a PosixPath to a string.

DerSchinken avatar Mar 04 '24 14:03 DerSchinken