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

Make importlib.machinery class handle os.PathLike path #74433

Open
mlouielu mannequin opened this issue May 3, 2017 · 6 comments
Open

Make importlib.machinery class handle os.PathLike path #74433

mlouielu mannequin opened this issue May 3, 2017 · 6 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-importlib

Comments

@mlouielu
Copy link
Mannequin

mlouielu mannequin commented May 3, 2017

BPO 30247
Nosy @brettcannon, @ncoghlan, @ericsnowcurrently, @serhiy-storchaka, @mlouielu
PRs
  • bpo-30247: Add os.PathLike handle for finder and loader #1422
  • 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 2017-05-03.07:42:38.788>
    labels = ['3.7', 'library']
    title = 'Make importlib.machinery class handle os.PathLike path'
    updated_at = <Date 2017-05-05.06:20:02.026>
    user = 'https://github.com/mlouielu'

    bugs.python.org fields:

    activity = <Date 2017-05-05.06:20:02.026>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2017-05-03.07:42:38.788>
    creator = 'louielu'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30247
    keywords = []
    message_count = 6.0
    messages = ['292856', '292857', '292899', '292926', '293041', '293045']
    nosy_count = 5.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'eric.snow', 'serhiy.storchaka', 'louielu']
    pr_nums = ['1422']
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue30247'
    versions = ['Python 3.7']

    @mlouielu
    Copy link
    Mannequin Author

    mlouielu mannequin commented May 3, 2017

    Several importlib.machinery class has 'path' attribute, make it possible to handle os.PathLike.

    @mlouielu mlouielu mannequin added the 3.7 (EOL) end of life label May 3, 2017
    @mlouielu
    Copy link
    Mannequin Author

    mlouielu mannequin commented May 3, 2017

    Relate to bpo-29448

    @mlouielu mlouielu mannequin added the stdlib Python modules in the Lib dir label May 3, 2017
    @serhiy-storchaka
    Copy link
    Member

    I'm not sure it's a good idea. FileLoader, FileFinder, and other classes usually are a part of import machinery, they can have other restrictions on path type than general filesystem related functions. This change has a non-zero cost, it complicates the code and makes it slower. Do you have a use case for using these classes with a path different from None or str?

    @brettcannon
    Copy link
    Member

    The classes mentioned actually require that the path exist on the file system so there's no extra restrictions. As for cost, it's pretty cheap as a call to _os.fspath() is written in C and does an immediate type-check for str or bytes for the common-case (https://github.com/python/cpython/blob/master/Modules/posixmodule.c#L12099). These classes are also instantiated once typically and then cached so the cost is only paid once per object.

    As for use-cases, I've seen people directly instantiate these classes in user code and so supporting paths in a universal way like the rest of the stdlib would be good for consistency.

    @serhiy-storchaka
    Copy link
    Member

    If the path attribute can be None or list it looks to me that it isn't a filesystem path, and it may be incorrect to use os.fspath() with it. How this attribute is used? What wrong if left it a pathlib.Path?

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented May 5, 2017

    FileFinder only handles a single directory, and FileLoader only handles a single file, so their "path" attributes are paths in the "fspath" sense, rather than the "sys.path" or "PathFinder" sense.

    Perhaps it would be worth the hassle of migrating to "fspath" as the attribute and parameter names here?

    The attribute can be renamed without breaking backwards compatibility by also adding a "path" property that manipulates the renamed "fspath" attribute.

    Renaming the parameters would be a bit trickier, since we would need to allow the parameter to be supplied under either name for at least 3.7, so we'd need to do something like:

    1. Remove the current positional-or-keyword "path" parameter
    2. Add keyword-only "path" and "fspath" parameters that default to None
    3. Accept arbitrary additional positional arguments (which is already the case for FileFinder)
    4. If both of the new keyword-only parameters are None, then extract "fspath" from the tuple of positional parameters (i.e. by doing "fspath, *loader_details loader_details" in FileFinder, and "fspath, * = args" in the FileLoader classes

    At a documentation level, this would just be described as the parameter name being "fspath", and a versionchanged note for 3.7 saying that the parameter name changed from "path" to "fspath" and the old "path' name is still supported as a keyword argument for backwards compatibility reasons.

    If we actively deprecated the old names, then the deprecation warnings would live in the access function definitions for the "path" attribute, and in the case where the "path" keyword argument was non-None for the renamed parameters.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-importlib
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants