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

Documentation/implementation out of sync for IO #67043

Closed
viraptor mannequin opened this issue Nov 12, 2014 · 15 comments
Closed

Documentation/implementation out of sync for IO #67043

viraptor mannequin opened this issue Nov 12, 2014 · 15 comments
Assignees
Labels
topic-IO type-bug An unexpected behavior, bug, or error

Comments

@viraptor
Copy link
Mannequin

viraptor mannequin commented Nov 12, 2014

BPO 22854
Nosy @pitrou, @benjaminp, @vadmium, @serhiy-storchaka
Files
  • UnsupportedOperation.patch
  • UnsupportedOperation.v2.patch
  • UnsupportedOperation.v3.patch: Change to OSError
  • 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/vadmium'
    closed_at = <Date 2016-03-31.23:54:06.363>
    created_at = <Date 2014-11-12.15:06:27.021>
    labels = ['type-bug', 'expert-IO']
    title = 'Documentation/implementation out of sync for IO'
    updated_at = <Date 2016-03-31.23:54:06.362>
    user = 'https://bugs.python.org/viraptor'

    bugs.python.org fields:

    activity = <Date 2016-03-31.23:54:06.362>
    actor = 'martin.panter'
    assignee = 'martin.panter'
    closed = True
    closed_date = <Date 2016-03-31.23:54:06.363>
    closer = 'martin.panter'
    components = ['IO']
    creation = <Date 2014-11-12.15:06:27.021>
    creator = 'viraptor'
    dependencies = []
    files = ['37520', '42194', '42301']
    hgrepos = []
    issue_num = 22854
    keywords = ['patch']
    message_count = 15.0
    messages = ['231079', '231080', '231095', '232989', '261728', '261942', '261953', '262503', '262506', '262690', '262691', '262692', '262693', '262694', '262708']
    nosy_count = 7.0
    nosy_names = ['pitrou', 'benjamin.peterson', 'stutzbach', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'viraptor']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22854'
    versions = ['Python 3.5', 'Python 3.6']

    @viraptor
    Copy link
    Mannequin Author

    viraptor mannequin commented Nov 12, 2014

    The docstring on for fileno() method says:

    "An IOError is raised if the IO object does not use a file descriptor."

    In reality, UnsupportedOperation is raised instead:

    : io.StringIO().fileno()
    UnsupportedOperation: fileno
    

    @viraptor viraptor mannequin added topic-IO type-bug An unexpected behavior, bug, or error labels Nov 12, 2014
    @viraptor
    Copy link
    Mannequin Author

    viraptor mannequin commented Nov 12, 2014

    Just in case: yes, UnsupportedOperation is an IOError - but shouldn't docstring here be more specific?

    @vadmium
    Copy link
    Member

    vadmium commented Nov 12, 2014

    Similarly for the readable(), seekable() and writable() documentation

    @vadmium
    Copy link
    Member

    vadmium commented Dec 21, 2014

    Some of the docstrings already mention UnsupportedOperation. This patch updates the rest of the documentation. Also adds some tests to verify this on all the concrete classes I could think of. Some discoveries in the process:

    • BufferedWriter.readable() and BufferedReader.writable() could return True depending on the underlying raw stream. Fixed to always return False.
    • Removed a branch in a test case that assumed BufferedReader.close() did not call flush(), but never activated due to the above writable() bug
    • seek(), tell() and truncate() do not raise UnsupportedOperation, despite seekable() == False, at least for a pipe. Adjusted doc strings.

    @serhiy-storchaka
    Copy link
    Member

    Nice patch. But it isn't applied cleanly on current tip (perhaps due to Argument Clinic). Added comments and questions on Rietveld.

    @vadmium
    Copy link
    Member

    vadmium commented Mar 18, 2016

    Thanks for looking at this Serhiy. Here is patch v2, merged with 3.6 branch; some doc string changes were redundant with upstream Arg Clinic changes.

    However looking at this again, I think we should be cautious changing the documented exceptions for the base classes, since that is changing the API. E.g. perhaps it would be safer to leave the exception for fileno() as OSError (aka IOError). It it actually causing a problem? Or the API changes should be reserved for 3.6 only.

    @serhiy-storchaka
    Copy link
    Member

    I have doubts about changing OSError to UnsupportedOperation in the documentation of readable() and writable(). Some implementations can try to do an IO operation without checking readable() and writable() flags.

    I have lesser doubt about using half-closed pipes in tests. May be it is safe, I don't know.

    @vadmium
    Copy link
    Member

    vadmium commented Mar 27, 2016

    Okay let’s document fileno(), read, write and seek operations in the base classes as raising OSError then. This effectively rejects the OP (Stanislaw’s) view that the exception should be more specific.

    In patch v3, I changed everything over to say OSError is raised. I also added a background thread to drain the pipe writer. And I removed a test_no_fileno() method which was having an “existential crisis”. :)

    @serhiy-storchaka
    Copy link
    Member

    LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2016

    New changeset dc9e5f09ac0c by Martin Panter in branch '3.5':
    Issue bpo-22854: Clarify documentation about UnsupportedOperation and add tests
    https://hg.python.org/cpython/rev/dc9e5f09ac0c

    New changeset c27e9dcad1a3 by Martin Panter in branch 'default':
    Issue bpo-22854: Merge UnsupportedOperation fixes from 3.5
    https://hg.python.org/cpython/rev/c27e9dcad1a3

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2016

    New changeset 3d9d9ca75a31 by Martin Panter in branch '2.7':
    Issue bpo-22854: fileno() is always required in IOBase; remove test
    https://hg.python.org/cpython/rev/3d9d9ca75a31

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2016

    New changeset fb10d1f5016e by Martin Panter in branch 'default':
    Issue bpo-22854: Skip pipe seekable() tests on Windows
    https://hg.python.org/cpython/rev/fb10d1f5016e

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2016

    New changeset 66765a49465f by Martin Panter in branch '3.5':
    Issue bpo-22854: Skip pipe seek tests on Windows
    https://hg.python.org/cpython/rev/66765a49465f

    New changeset 3b7811b58a1f by Martin Panter in branch 'default':
    Issue bpo-22854: Merge Windows pipe skipping from 3.5
    https://hg.python.org/cpython/rev/3b7811b58a1f

    @vadmium
    Copy link
    Member

    vadmium commented Mar 31, 2016

    I gave up on porting the fix to 2.7. Python 3 raises UnsupportedOperation (which inherits ValueError) in many cases due to bpo-9293, but Python 2 raises IOError (which does not inherit ValueError). Changing how BufferedWriter.read() etc work in Python 2 would break test_io_after_close(). Also, none of the doc strings in Python 2 need fixing.

    Also according to the buildbots, Windows can seek in pipes. Or at least tell() doesn’t raise an exception. So I skipped the seek testing on Windows.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 31, 2016

    New changeset b3c79e0ba477 by Martin Panter in branch '3.5':
    Issue bpo-22854: Fix logic for skipping test
    https://hg.python.org/cpython/rev/b3c79e0ba477

    New changeset de8412dc477e by Martin Panter in branch 'default':
    Issue bpo-22854: Merge test fix from 3.5
    https://hg.python.org/cpython/rev/de8412dc477e

    @vadmium vadmium closed this as completed Mar 31, 2016
    @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
    topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants