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

[Yinon]

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

[ericvsmith]

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

[Yinon]

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

[ericvsmith]

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.

[merwok]

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

[ericvsmith]

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]

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 :(.

[berkerpeksag]

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

[ericvsmith]

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]

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]

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

[python-dev]

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]

Done. Thanks for the reviews!