GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111835: Add seekable method to mmap.mmap by corona10 · Pull Request #111852 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Nov 8, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Add seekable method for mmap.mmap #111835

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

technillogue reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@corona10
pythongh-111835: Add seekable method to mmap.mmap
4bac18a
@bedevere-app bedevere-app Bot added the awaiting core review label Nov 8, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Nov 8, 2023
Add seekable method for mmap.mmap #111835
Closed
corona10 added 2 commits November 8, 2023 23:35
@corona10
Add whats news
7e69d94
@corona10
Update mmap.rst
ebc837d
@corona10
corona10 requested review from serhiy-storchaka and vstinner November 8, 2023 14:39
@corona10

corona10 commented Nov 8, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@serhiy-storchaka cc @vstinner
As the issue reporter said, they can use seekable by subclassing mmap itself.
But this change definitely helpful to people who need to use library which checks that the file object supports seekable() method.
(I don't want to say it must be added but it is good to be added)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 added 3 commits November 8, 2023 14:51
@corona10
pythongh-111835: Add seekable method to mmap.mmap
d699f98
@corona10
Add whats news
c56a810
@corona10
Update mmap.rst
9748be9
@corona10
corona10 force-pushed the gh-111835 branch from ebc837d to 9748be9 Compare November 8, 2023 14:51
@corona10
Merge branch 'main' into pythongh-111835
8a12113
vstinner
vstinner approved these changes Nov 8, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/mmap.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Nov 8, 2023
corona10 added 2 commits November 9, 2023 00:21
@corona10
Address Victor's review
9021e4e
@corona10
Merge remote-tracking branch 'origin/pythongh-111835' into pythongh-1…
1b8d6cc
…11835
@serhiy-storchaka

serhiy-storchaka commented Nov 8, 2023

Copy link
Copy Markdown
Member

mmap.seek() always return None. It can make mmap not appropriate substitution for files. Adding seekable() method can break existing code.

corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10
Update mmap.seek() to return the current position
fa01823
@corona10 corona10 mentioned this pull request Nov 8, 2023
pythongh-111835: Add seekable method to mmap.mmap #111865
Closed
@corona10
Update documentation
db9387e
@corona10

corona10 commented Nov 8, 2023

Copy link
Copy Markdown
Member Author

mmap.seek() always return None. It can make mmap not appropriate substitution for files. Adding seekable() method can break existing code.

I left a comment for this.
#111835 (comment)
But I'm not sure how it will break the user's code even if we update seek() to return the current position.
I can not guess how people are doing something with None value.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 added 4 commits November 9, 2023 07:20
@corona10
Remove white space
08e9966
@corona10
Add credit
d576c8c
@corona10
Update credit
0ed19e4
@corona10
Use PyLong_FromSsize_t
55f47cb
serhiy-storchaka
serhiy-storchaka reviewed Nov 9, 2023
View reviewed changes
Comment thread Doc/library/mmap.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.13.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_mmap.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@corona10
Address code review
ff11b75
@corona10
corona10 requested a review from serhiy-storchaka November 9, 2023 07:54
vstinner
vstinner reviewed Nov 9, 2023
View reviewed changes
Comment thread Modules/mmapmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Nov 9, 2023
View reviewed changes
Comment thread Doc/whatsnew/3.13.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Nov 9, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-11-08-23-32-03.gh-issue-111835.ufFiuW.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@corona10
Address code review
0c4245b
serhiy-storchaka
serhiy-storchaka approved these changes Nov 9, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
vstinner
vstinner approved these changes Nov 9, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@corona10
corona10 merged commit 6046aec into python:main Nov 9, 2023
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 9, 2023
@corona10

corona10 commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

@serhiy-storchaka @vstinner
Thank you for the reviews!!

vstinner reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@corona10
corona10 deleted the gh-111835 branch November 9, 2023 11:13
@technillogue technillogue mentioned this pull request Jan 21, 2024
Awkward wording in GH-111835 whatsnew entry #114364
Closed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@corona10 @aisk
pythongh-111835: Add seekable method to mmap.mmap (pythongh-111852)
3706873
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@corona10 @Glyphack
pythongh-111835: Add seekable method to mmap.mmap (pythongh-111852)
1ffbd09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@corona10 @serhiy-storchaka @vstinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.