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

GzipFile doesn't have peek() #54171

Closed
pitrou opened this issue Sep 27, 2010 · 12 comments
Closed

GzipFile doesn't have peek() #54171

pitrou opened this issue Sep 27, 2010 · 12 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented Sep 27, 2010

BPO 9962
Nosy @pitrou
Files
  • gzippeek.patch
  • gzippeek2.patch
  • gzipfixup.patch
  • 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 = None
    closed_at = <Date 2010-10-04.21:55:55.125>
    created_at = <Date 2010-09-27.21:38:34.119>
    labels = ['type-feature', 'library']
    title = "GzipFile doesn't have peek()"
    updated_at = <Date 2010-10-04.21:55:55.123>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2010-10-04.21:55:55.123>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-10-04.21:55:55.125>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2010-09-27.21:38:34.119>
    creator = 'pitrou'
    dependencies = []
    files = ['19038', '19053', '19074']
    hgrepos = []
    issue_num = 9962
    keywords = ['patch']
    message_count = 12.0
    messages = ['117476', '117491', '117552', '117594', '117706', '117752', '117766', '117782', '117785', '117786', '117788', '117986']
    nosy_count = 2.0
    nosy_names = ['pitrou', 'nirai']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue9962'
    versions = ['Python 3.2']

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 27, 2010

    GzipFile claims to implement BufferedIOBase but doesn't have peek().

    @pitrou pitrou added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Sep 27, 2010
    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 27, 2010

    Here is a first patch, tests still need to be written.

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 28, 2010

    Same patch with tests.

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 29, 2010

    Committed in r85100.

    @pitrou pitrou closed this as completed Sep 29, 2010
    @nirai
    Copy link
    Mannequin

    nirai mannequin commented Sep 30, 2010

    Hi Antoine,

    BufferedIOBase is not documented to have peek():
    http://docs.python.org/dev/py3k/library/io.html

    Small note about patch:

    1. IOError string says "read() on write-only...", should be "peek() on write-only..." ?
    2. Should be min() in self._read(max(self.max_read_chunk, n))

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 30, 2010

    Hir Nir,

    BufferedIOBase is not documented to have peek():
    http://docs.python.org/dev/py3k/library/io.html

    Ah, you're right.

    Small note about patch:

    1. IOError string says "read() on write-only...", should be "peek() on write-only..." ?

    Indeed.

    1. Should be min() in self._read(max(self.max_read_chunk, n))

    Actually, I think I should have reproduced the algorithm in read(),
    where there's a read_size distinct from the size requested by the user.

    Thanks for the review, it looks like I should have waited a bit before
    committing :)

    @pitrou pitrou reopened this Sep 30, 2010
    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 30, 2010

    Here is a patch fixing these issues.

    @nirai
    Copy link
    Mannequin

    nirai mannequin commented Oct 1, 2010

    Should be min(n, 1024) instead of max(...)

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 1, 2010

    Should be min(n, 1024) instead of max(...)

    Well, no, because we want to buffer a non-trivial amount of bytes for
    the next accesses. So, if n < 1024, buffer at least 1024 bytes.

    @nirai
    Copy link
    Mannequin

    nirai mannequin commented Oct 1, 2010

    Right, I missed the change from self.max_read_chunk to 1024 (read_size). Should not peek() limit to self.max_read_chunk as read() does?

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 1, 2010

    Right, I missed the change from self.max_read_chunk to 1024
    (read_size). Should not peek() limit to self.max_read_chunk as read()
    does?

    This is used for the chunking of huge reads, but for peek():

    1. there is no chunking (peek() should do at most one raw read)
    2. huge reads are not really the use case peek() is intended for

    @pitrou
    Copy link
    Member Author

    pitrou commented Oct 4, 2010

    I've committed the improvements in r85221. Thank you!

    @pitrou pitrou closed this as completed Oct 4, 2010
    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant