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

add os.fspath() #71373

Closed
ethanfurman opened this issue Jun 2, 2016 · 45 comments
Closed

add os.fspath() #71373

ethanfurman opened this issue Jun 2, 2016 · 45 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@ethanfurman
Copy link
Member

ethanfurman commented Jun 2, 2016

BPO 27186
Nosy @brettcannon, @ethanfurman, @vadmium, @serhiy-storchaka, @JelleZijlstra, @buchuki, @erikjanss
Files
  • issue27186-fsencode.buchuki.patch
  • issue27186.patch: patch to add os.fspath and PyOS_FsPath
  • issue27186-DirEntry-fspath.patch: adds fspath support to os.DirEntry
  • issue27186-pathlib.buchuki.patch
  • issue27186-glossary.buchuki.patch: path-like glossary entry
  • issue27186-DirEntry-fspath.patch: patch to add fspath support to os.DirEntry
  • issue27186-os_path_t.patch: patch for the path_t converter in os
  • 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/brettcannon'
    closed_at = <Date 2016-06-24.19:28:56.707>
    created_at = <Date 2016-06-02.18:22:21.760>
    labels = ['type-feature']
    title = 'add os.fspath()'
    updated_at = <Date 2018-08-13.23:04:39.687>
    user = 'https://github.com/ethanfurman'

    bugs.python.org fields:

    activity = <Date 2018-08-13.23:04:39.687>
    actor = 'erikjanss'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2016-06-24.19:28:56.707>
    closer = 'brett.cannon'
    components = []
    creation = <Date 2016-06-02.18:22:21.760>
    creator = 'ethan.furman'
    dependencies = []
    files = ['43124', '43125', '43128', '43133', '43155', '43205', '43215']
    hgrepos = []
    issue_num = 27186
    keywords = ['patch']
    message_count = 45.0
    messages = ['266971', '266979', '266982', '266984', '267004', '267105', '267127', '267285', '267293', '267295', '267299', '267301', '267306', '267316', '267322', '267327', '267328', '267336', '267348', '267429', '267465', '268060', '268062', '268070', '268146', '268158', '268329', '268393', '268394', '268399', '268414', '268415', '268433', '268457', '268628', '269204', '269205', '269207', '269208', '269209', '269795', '269800', '323225', '323228', '323495']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'ethan.furman', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'JelleZijlstra', 'buchuki', 'erikjanss']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27186'
    versions = ['Python 3.6']

    @ethanfurman ethanfurman added the type-feature A feature request or enhancement label Jun 2, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 2, 2016

    New changeset 59a52a9dd0dc by Ethan Furman in branch 'default':
    bpo-27186 -- initial docs, tests, and python version of os.fspath
    https://hg.python.org/cpython/rev/59a52a9dd0dc

    @buchuki
    Copy link
    Mannequin

    buchuki mannequin commented Jun 2, 2016

    Make os.fsencode and os.fsdecode able to accept a PathLike by calling into fspath. Additionally adds test for PathLike objects.

    @buchuki
    Copy link
    Mannequin

    buchuki mannequin commented Jun 2, 2016

    Test __fspath__ returning bytes as well.

    @JelleZijlstra
    Copy link
    Member

    JelleZijlstra commented Jun 2, 2016

    This patch adds the C implementation (copied from the PEP).

    Some notes:

    • I added a new .h file in Include/ because there didn't seem to be an obvious existing place to put it.
    • There was some uncertainty about whether we should Py_INCREF the string or bytes returned when a string/bytes is passed in. Running test -l and -L works with the Py_INCREF in and we're adding a new reference to the module, so I think this is correct.

    @buchuki
    Copy link
    Mannequin

    buchuki mannequin commented Jun 3, 2016

    This patch adds fspath protocol and constructor initialization to pathlib.Path.

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 3, 2016

    Note: My schedule changed -- I won't be at the sprints today, but tomorrow. I'll finish reviewing these patches and commit this afternoon if no one beats me to it.

    @buchuki
    Copy link
    Mannequin

    buchuki mannequin commented Jun 3, 2016

    Adding a glossary entry for path-like. The references are all correct except:

    :class:`os.PathLike` doesn't link to anything because PathLike hasn't been added to the os module yet. Similarly, :meth:`__fspath__` does not link to anything, although I think this is acceptable.

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 4, 2016

    Dusty: The pathlib tests looks like it's only testing the PurePath implementation; while this /should/ be sufficient, please add tests for the other classes as well.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 4, 2016

    New changeset 780cbe18082e by Ethan Furman in branch 'default':
    bpo-27186: add C version of os.fspath(); patch by Jelle Zijlstra
    https://hg.python.org/cpython/rev/780cbe18082e

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 4, 2016

    New changeset 00991aa5fdb5 by Ethan Furman in branch 'default':
    bpo-27182: update fsencode and fsdecode for os.path(); patch by Dusty Phillips
    https://hg.python.org/cpython/rev/00991aa5fdb5

    (had wrong issue # in commit)

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 4, 2016

    Jelle: the DirEntry patch looks good so far, but it needs a test for a bytes path.

    @JelleZijlstra
    Copy link
    Member

    JelleZijlstra commented Jun 4, 2016

    Thanks for reviewing. This patch adds tests with a bytes DirEntry.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 4, 2016

    New changeset e672cf63d08a by Ethan Furman in branch 'default':
    bpo-27186: add PathLike ABC
    https://hg.python.org/cpython/rev/e672cf63d08a

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 4, 2016

    New changeset 76b2ddaee6bd by Ethan Furman in branch 'default':
    bpo-27186: fix fsencode/fsdecode and update tests; patch by Jelle Zijlstra
    https://hg.python.org/cpython/rev/76b2ddaee6bd

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 4, 2016

    New changeset 254125a265d2 by Ethan Furman in branch 'default':
    bpo-27186: add open/io.open; patch by Jelle Zijlstra
    https://hg.python.org/cpython/rev/254125a265d2

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 4, 2016

    Jelle: We still need os.open if you would like to work on that. :)

    @JelleZijlstra
    Copy link
    Member

    JelleZijlstra commented Jun 4, 2016

    Sure, I'll do that too. Now that os.PathLike and PyOS_FSPath exist, it should also be possible to add support to os.path.

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 4, 2016

    os.path is actually two different modules: posixpath.py and ntpath.py

    posixpath.py is being tracked in bpo-26027

    ntpath.py is being tracked in bpo-27184

    @JelleZijlstra
    Copy link
    Member

    JelleZijlstra commented Jun 4, 2016

    This patch adds support for fspath to a number of functions in the os module by augmenting the path_t argument converter.

    The tests only cover a subset of the functions that use path_t, because some (e.g., unlink) have destructive side effects.

    @JelleZijlstra
    Copy link
    Member

    JelleZijlstra commented Jun 5, 2016

    I moved my patch for the path_t converter to bpo-26027, which already covers posixmodule.c. I'm creating bpo-27231 to track adding os.fspath() support to posixpath.py. With that, I think this issue is done.

    @buchuki
    Copy link
    Mannequin

    buchuki mannequin commented Jun 5, 2016

    Ethan: Can you clarify what you mean by "testing the other classes"? PureWindowsPath and PurePosixPath are tested by extension of _BasePurePathTest. So I think you mean that _BasePathTest also needs testing, but I don't see anything in there that would mirror the constructor testing that is happening in PurePath.

    @brettcannon
    Copy link
    Member

    brettcannon commented Jun 9, 2016

    Re-opening as there are several patches that got put in here that have not been applied (e.g. glossary entry, os.DirEntry, etc.).

    @brettcannon brettcannon reopened this Jun 9, 2016
    @brettcannon brettcannon self-assigned this Jun 9, 2016
    @brettcannon
    Copy link
    Member

    brettcannon commented Jun 9, 2016

    I'm starting to catch up on everything you all did for PEP-519 and I wanted to say thanks! It looks like everything that needs to be done has been committed, has a patch, or just needs docs. The only thing I needed to do post-commit is tweak the docstrings to be PEP-8 compatible and reword some documention and docstrings.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 9, 2016

    New changeset cec1f00c538d by Brett Cannon in branch 'default':
    Issue bpo-27186: Document PyOS_FSPath().
    https://hg.python.org/cpython/rev/cec1f00c538d

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 10, 2016

    New changeset a5a013ca5687 by Brett Cannon in branch 'default':
    Issue bpo-27186: Add os.PathLike support to pathlib.
    https://hg.python.org/cpython/rev/a5a013ca5687

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 10, 2016

    New changeset 5a62d682636e by Brett Cannon in branch 'default':
    Issue bpo-27186: Add os.PathLike support to DirEntry
    https://hg.python.org/cpython/rev/5a62d682636e

    @vadmium
    Copy link
    Member

    vadmium commented Jun 12, 2016

    Test_fspath_protocol_bytes() (added in revision 5a62d682636e) fails on Windows:

    http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/7777/steps/test/logs/stdio
    ======================================================================
    ERROR: test_fspath_protocol_bytes (test.test_os.TestScandir)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_os.py", line 2958, in test_fspath_protocol_bytes
        bytes_entry = self.create_file_entry(name=bytes_filename)
      File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_os.py", line 2932, in create_file_entry
        return self.get_entry(os.path.basename(filename))
      File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_os.py", line 2923, in get_entry
        entries = list(os.scandir(path))
    TypeError: os.scandir() doesn't support bytes path on Windows, use Unicode instead

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 12, 2016

    New changeset 6a35aa1995ab by Brett Cannon in branch 'default':
    Issue bpo-27186: skip bytes path test for os.scandir() on Windows
    https://hg.python.org/cpython/rev/6a35aa1995ab

    @brettcannon
    Copy link
    Member

    brettcannon commented Jun 12, 2016

    Thanks for the notice, Martin!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 12, 2016

    New changeset 1ccd6196115a by Brett Cannon in branch 'default':
    Issue bpo-27186: add Include/osmodule.h to the proper build rules
    https://hg.python.org/cpython/rev/1ccd6196115a

    @vadmium
    Copy link
    Member

    vadmium commented Jun 13, 2016

    Should the skip logic perhaps check for sys.platform == "win32" instead? The buildbots are still failing.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 13, 2016

    New changeset f384c5c14488 by Martin Panter in branch 'default':
    Issue bpo-27186: Skip scandir(bytes) test with os.name == "nt"
    https://hg.python.org/cpython/rev/f384c5c14488

    @vadmium
    Copy link
    Member

    vadmium commented Jun 13, 2016

    I went with os.name == "nt", which is what other scandir() tests use. I’m not sure there is a practical different. Anyway the buildbots seem happier now.

    @brettcannon
    Copy link
    Member

    brettcannon commented Jun 13, 2016

    Thanks for catching my screw-up, Martin; I misread the checks in the file by noticing the "nt" bit but not picking up it was comparing against sys.name instead of sys.platform.

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 15, 2016

    os.fspath() will be changed to ensure the output of calling obj.__fspath__() is a str or bytes object.

    So the final behavior of calling os.fspath() will be to return a str or bytes or to raise an exception.

    I'll update the code for this change, as well is the places in the stdlib that do/should be using os.fspath().

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 24, 2016

    New changeset ea7b6a7827a4 by Brett Cannon in branch 'default':
    Issue bpo-27186: Update os.fspath()/PyOS_FSPath() to check the return
    https://hg.python.org/cpython/rev/ea7b6a7827a4

    @brettcannon
    Copy link
    Member

    brettcannon commented Jun 24, 2016

    And now that I have updated os.fspath() I realize that Ethan said he was going to implement the update. :( If you started on the work, Ethan, I'm really sorry for duplicating your (potential) work.

    I'm going to commit Dusty's glossary term next and then go through and update the docs to use it. At that point that will leave updating os.path as the last explicit step in PEP-519 that needs to be short of updating "What's New". We're getting there. :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 24, 2016

    New changeset 9c57178f13dc by Brett Cannon in branch 'default':
    Issue bpo-27186: Define what a "path-like object" is.
    https://hg.python.org/cpython/rev/9c57178f13dc

    @brettcannon
    Copy link
    Member

    brettcannon commented Jun 24, 2016

    I think with the glossary change, this issue is done! Thanks everyone for the help with getting this far!

    Now on to os.path...

    @ethanfurman
    Copy link
    Member Author

    ethanfurman commented Jun 24, 2016

    Brett, no worries. My time has been extremely limited.

    I'll get the other locations in the stdlib fixed sometime in the next two months if no one beats me to it.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Jul 4, 2016

    Maybe merge Include/osmodule.h and Modules/posixmodule.h?

    @brettcannon
    Copy link
    Member

    brettcannon commented Jul 4, 2016

    I'm fine with merging the two files if you want to do it. Build rules will also need updating if they do get merged.

    @erikjanss
    Copy link
    Mannequin

    erikjanss mannequin commented Aug 6, 2018

    is there a particular reason for PyOS_FSPath to live in posixmodule.c

    since Objects/unicodeobject.c uses this function, this makes it not possible to compile Python without a posixmodule.

    this makes it difficult to compile a 'core' python on a new platform, since implementing the posixmodule on a new platform is not trivial.

    also the documentation on porting mentions that one should first port python without a posixmodule.

    @brettcannon
    Copy link
    Member

    brettcannon commented Aug 6, 2018

    On Mon, Aug 6, 2018, 13:56 Erik Janssens, <report@bugs.python.org> wrote:

    Erik Janssens <erik.janssens@conceptive.be> added the comment:

    is there a particular reason for PyOS_FSPath to live in posixmodule.c

    It's there because the C API for the os module is kept in that module.

    @erikjanss
    Copy link
    Mannequin

    erikjanss mannequin commented Aug 13, 2018

    thank you for the info ... I'll have a look at making the posix module easier to port ...

    @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
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants