X Tutup
The Wayback Machine - https://web.archive.org/web/20240502115245/https://github.com/python/cpython/issues/90149
Skip to content
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.

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

Improve ambiguous docstrings in pkgutil #90149

Open
khock mannequin opened this issue Dec 5, 2021 · 9 comments
Open

Improve ambiguous docstrings in pkgutil #90149

khock mannequin opened this issue Dec 5, 2021 · 9 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes docs Documentation in the Doc dir easy type-feature A feature request or enhancement

Comments

@khock
Copy link
Mannequin

khock mannequin commented Dec 5, 2021

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']

@khock
Copy link
Mannequin Author

khock mannequin commented Dec 5, 2021

# 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: duo-labs/parliament#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

@khock khock mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.10 only security fixes 3.11 only security fixes 3.9 only security fixes labels Dec 5, 2021
@khock khock mannequin assigned docspython Dec 5, 2021
@khock khock mannequin added 3.8 only security fixes docs Documentation in the Doc dir 3.10 only security fixes type-feature A feature request or enhancement 3.11 only security fixes 3.9 only security fixes labels Dec 5, 2021
@khock khock mannequin assigned docspython Dec 5, 2021
@khock khock mannequin added docs Documentation in the Doc dir type-feature A feature request or enhancement labels Dec 5, 2021
@iritkatriel iritkatriel added easy and removed 3.7 (EOL) end of life 3.8 only security fixes labels Jan 14, 2022
@slateny
Copy link
Contributor

slateny commented Feb 24, 2022

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

@khock
Copy link
Mannequin Author

khock mannequin commented Feb 24, 2022

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.

@slateny
Copy link
Contributor

slateny commented Feb 25, 2022

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?

@barneygale
Copy link
Mannequin

barneygale mannequin commented Feb 26, 2022

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

@khock
Copy link
Mannequin Author

khock mannequin commented Feb 28, 2022

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

@slateny
Copy link
Contributor

slateny commented Mar 1, 2022

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.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@safwansamsudeen
Copy link

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

@DerSchinken
Copy link
Contributor

DerSchinken commented Mar 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes docs Documentation in the Doc dir easy type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants
X Tutup