➜
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2014-04-30 08:49 byYinon, last changed2022-04-11 14:58 byadmin. This issue is nowclosed.
| 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)* | 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)* | 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" inhttp://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)* | 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)* | 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.htmlso 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)* | 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)* | Date: 2014-05-18 00:48 | |
Here is a patch to deprecate the shutil.abspath function. | |||
| msg221626 -(view) | Author: Eric V. Smith (eric.smith)* | 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)* | 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-interfacessays:"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)* | 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 changesetab369d809200 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)* | Date: 2014-09-18 02:11 | |
Done. Thanks for the reviews! | |||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:58:03 | admin | set | github: 65590 |
| 2014-09-18 02:11:52 | berker.peksag | set | status: open -> closed resolution: fixed messages: +msg227023 stage: needs patch -> resolved |
| 2014-09-18 02:10:42 | python-dev | set | nosy: +python-dev messages: +msg227022 |
| 2014-08-23 17:13:55 | berker.peksag | set | assignee:berker.peksag |
| 2014-08-23 16:42:13 | serhiy.storchaka | set | nosy: +serhiy.storchaka messages: +msg225752 stage: commit review -> needs patch |
| 2014-06-26 17:56:21 | eric.smith | set | messages: +msg221628 |
| 2014-06-26 17:39:41 | eric.smith | set | messages: +msg221626 |
| 2014-06-26 17:30:15 | berker.peksag | set | stage: patch review -> commit review |
| 2014-05-18 00:48:22 | berker.peksag | set | files: +issue21391.diff nosy: +berker.peksag messages: +msg218725 |
| 2014-05-17 13:46:14 | r.david.murray | set | nosy: +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:08 | eric.smith | set | messages: +msg217773 |
| 2014-05-02 17:59:39 | eric.araujo | set | nosy: +eric.araujo messages: +msg217769 |
| 2014-05-01 11:48:21 | eric.smith | set | type: behavior stage: patch review |
| 2014-05-01 11:47:49 | eric.smith | set | messages: +msg217688 |
| 2014-05-01 11:40:35 | Yinon | set | files: +shutil.patch keywords: +patch messages: +msg217686 |
| 2014-04-30 09:51:46 | eric.smith | set | nosy: +eric.smith messages: +msg217588 |
| 2014-04-30 08:49:28 | Yinon | create | |