classification
Title: shutil uses both os.path.abspath and an 'import from' of abspath
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: berker.peksag Nosy List: Yinon, berker.peksag, eric.araujo, eric.smith, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-04-30 08:49 by Yinon, last changed 2014-09-18 02:11 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
shutil.patch Yinon, 2014-05-01 11:40 review
issue21391.diff berker.peksag, 2014-05-18 00:48 review
Messages (12)
msg217588 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2014-04-30 09:51
If you meant to supply a patch, it is missing. And in any event, you need to describe the issue.
msg217686 - (view) Author: Yinon Ehrlich (Yinon) Date: 2014-05-01 11:40
Use the 'abspath' shortcut instead of 'os.path.abspath'
See the attached patch (sorry, forgot to attach before)
msg217688 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2014-05-01 11:47
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.
msg217769 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-05-02 17:59
IMO either change would not improve the code at all.  Suggest closing this.
msg217773 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2014-05-02 18:29
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).
msg218702 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-05-17 13:46
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 :(.
msg218725 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-05-18 00:48
Here is a patch to deprecate the shutil.abspath function.
msg221626 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2014-06-26 17:39
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.
msg221628 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2014-06-26 17:56
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.
msg225752 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-23 16:42
I'm for get rid of "from import" without deprecation. Definitely this is not part of API.
msg227022 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-18 02:10
New changeset ab369d809200 by Berker Peksag in branch 'default':
Issue #21391: Use os.path.abspath in the shutil module.
https://hg.python.org/cpython/rev/ab369d809200
msg227023 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2014-09-18 02:11
Done. Thanks for the reviews!
History
Date User Action Args
2014-09-18 02:11:52berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg227023

stage: needs patch -> resolved
2014-09-18 02:10:42python-devsetnosy: + python-dev
messages: + msg227022
2014-08-23 17:13:55berker.peksagsetassignee: berker.peksag
2014-08-23 16:42:13serhiy.storchakasetnosy: + serhiy.storchaka

messages: + msg225752
stage: commit review -> needs patch
2014-06-26 17:56:21eric.smithsetmessages: + msg221628
2014-06-26 17:39:41eric.smithsetmessages: + msg221626
2014-06-26 17:30:15berker.peksagsetstage: patch review -> commit review
2014-05-18 00:48:22berker.peksagsetfiles: + issue21391.diff
nosy: + berker.peksag
messages: + msg218725

2014-05-17 13:46:14r.david.murraysetnosy: + r.david.murray

messages: + msg218702
title: PATCH: using the abspath shortcut in Lib/shutil -> shutil uses both os.path.abspath and an 'import from' of abspath
2014-05-02 18:29:08eric.smithsetmessages: + msg217773
2014-05-02 17:59:39eric.araujosetnosy: + eric.araujo
messages: + msg217769
2014-05-01 11:48:21eric.smithsettype: behavior
stage: patch review
2014-05-01 11:47:49eric.smithsetmessages: + msg217688
2014-05-01 11:40:35Yinonsetfiles: + shutil.patch
keywords: + patch
messages: + msg217686
2014-04-30 09:51:46eric.smithsetnosy: + eric.smith
messages: + msg217588
2014-04-30 08:49:28Yinoncreate