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.

Sign up for GitHub

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

Jump to bottom

Thanks! Mostly looks good. If you want to land this quickly, please do, but I'd appreciate a follow-up (no-test) to put back the comment. #19916

Closed
allisonkarlitskaya opened this issue Jan 31, 2024 · 2 comments
Closed

Thanks! Mostly looks good. If you want to land this quickly, please do, but I'd appreciate a follow-up (no-test) to put back the comment. #19916

allisonkarlitskaya opened this issue Jan 31, 2024 · 2 comments
Assignees
@allisonkarlitskaya

Comments

@allisonkarlitskaya
Copy link
Member

That comment is still current, other than the "we don't have .index" bit. But it's useful to explain the magic number 10 below, as well as splitting reads in between a number.

Originally posted by @martinpitt in #19270 (review)
@allisonkarlitskaya allisonkarlitskaya self-assigned this Jan 31, 2024
@allisonkarlitskaya
Copy link
Member Author

@martinpitt I actually just read this comment and it's definitely no longer relevant. We no longer slice anything, specifically because the part about not having memoryview.index() is no longer relevant. That means the performance concerns are also not relevant...

@martinpitt
Copy link
Member

OK, thanks for checking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@allisonkarlitskaya allisonkarlitskaya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@martinpitt @allisonkarlitskaya