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.

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

shutil uses both os.path.abspath and an 'import from' of abspath #65590

Closed
Yinon mannequin opened this issue Apr 30, 2014 · 12 comments
Closed

shutil uses both os.path.abspath and an 'import from' of abspath #65590

Yinon mannequin opened this issue Apr 30, 2014 · 12 comments
Assignees
@berkerpeksag
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@Yinon
Copy link
Mannequin

Yinon mannequin commented Apr 30, 2014

BPO 21391
Nosy @ericvsmith, @merwok, @bitdancer, @berkerpeksag, @serhiy-storchaka
Files
  • shutil.patch
  • issue21391.diff
    		•

    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 = 'https://github.com/berkerpeksag'
closed_at = <Date 2014-09-18.02:11:52.058>
created_at = <Date 2014-04-30.08:49:28.571>
labels = ['type-bug', 'library']
title = "shutil uses both os.path.abspath and an 'import from' of abspath"
updated_at = <Date 2014-09-18.02:11:52.057>
user = 'https://bugs.python.org/Yinon'

    bugs.python.org fields:

    activity = <Date 2014-09-18.02:11:52.057>
actor = 'berker.peksag'
assignee = 'berker.peksag'
closed = True
closed_date = <Date 2014-09-18.02:11:52.058>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2014-04-30.08:49:28.571>
creator = 'Yinon'
dependencies = []
files = ['35126', '35272']
hgrepos = []
issue_num = 21391
keywords = ['patch']
message_count = 12.0
messages = ['217588', '217686', '217688', '217769', '217773', '218702', '218725', '221626', '221628', '225752', '227022', '227023']
nosy_count = 7.0
nosy_names = ['eric.smith', 'Yinon', 'eric.araujo', 'r.david.murray', 'python-dev', 'berker.peksag', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21391'
versions = ['Python 3.5']
    @Yinon Yinon mannequin added the stdlib Python modules in the Lib dir label Apr 30, 2014
    @ericvsmith
    Copy link
    Member

    If you meant to supply a patch, it is missing. And in any event, you need to describe the issue.

    @Yinon
    Copy link
    Mannequin Author

    Yinon mannequin commented May 1, 2014

    Use the 'abspath' shortcut instead of 'os.path.abspath'
    See the attached patch (sorry, forgot to attach before)

    @ericvsmith
    Copy link
    Member

    Wouldn't it be better to switch uses of abspath to be os.path.abspath? os.path is used elsewhere in the file, after all.

    Brett added "from os.path import abspath" in http://hg.python.org/cpython/rev/686e5d38be42 but I think that import should be deleted and os.path.abspath used directly.

    @ericvsmith ericvsmith added the type-bug An unexpected behavior, bug, or error label May 1, 2014
    @merwok
    Copy link
    Member

    merwok commented May 2, 2014

    IMO either change would not improve the code at all. Suggest closing this.

    @ericvsmith
    Copy link
    Member

    I disagree. It took me longer than I'd like to admit to track down the file history and understand it. I'd like to prevent other people from having to try and understand why it works this way.

    On the other hand, it looks like people have discovered it:
    https://mail.python.org/pipermail/tutor/2012-August/090891.html
    so getting rid of it isn't so simple.

    If we are going to keep it, we should add a test for it (which actually might exist, I haven't checked yet).

    @bitdancer
    Copy link
    Member

    I'd prefer to get rid of it, otherwise we might get requests to add all the other os.path functions to the shutil namespace, and I don't think having that kind of "more than one way to do it" serves anyone. I suppose we'll have to deprecate it first if we do get rid of it :(.

    @bitdancer bitdancer changed the title PATCH: using the abspath shortcut in Lib/shutil shutil uses both os.path.abspath and an 'import from' of abspath May 17, 2014
    @berkerpeksag
    Copy link
    Member

    Here is a patch to deprecate the shutil.abspath function.

    @ericvsmith
    Copy link
    Member

    Shouldn't the existing calls to abspath() be changed to os.path.abspath()? Or are both patches meant to be applied? I don't think the first patch applies cleanly any more.

    In any event: the deprecation and test look good to me. So assuming we get rid of the import and get rid of direct calls to abspath(), I'm +1.

    @ericvsmith
    Copy link
    Member

    Now that I think about it, maybe we don't need a deprecation warning.

    http://legacy.python.org/dev/peps/pep-0008/#public-and-internal-interfaces

    says:

    "Imported names should always be considered an implementation detail. Other modules must not rely on indirect access to such imported names unless they are an explicitly documented part of the containing module's API, such as os.path or a package's __init__ module that exposes functionality from submodules."

    abspath isn't in __all__, so it's arguably not part of the public API, anyway.

    @serhiy-storchaka
    Copy link
    Member

    I'm for get rid of "from import" without deprecation. Definitely this is not part of API.

    @berkerpeksag berkerpeksag self-assigned this Aug 23, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 18, 2014

    New changeset ab369d809200 by Berker Peksag in branch 'default':
    Issue bpo-21391: Use os.path.abspath in the shutil module.
    https://hg.python.org/cpython/rev/ab369d809200

    @berkerpeksag
    Copy link
    Member

    Done. Thanks for the reviews!

    @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
    Assignees

    @berkerpeksag berkerpeksag

    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @berkerpeksag @bitdancer @ericvsmith @merwok @serhiy-storchaka