8000 gh-86809: Add support for HTTP Range header in HTTPServer by lyc8503 · Pull Request #118949 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-86809: Add support for HTTP Range header in HTTPServer #118949

8000
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
Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

gh-86809: Add support for HTTP Range header in HTTPServer #118949

wants to merge 24 commits into from

Conversation

lyc8503
Copy link
Contributor
@lyc8503 lyc8503 commented May 11, 2024 • 8000
edited by bedevere-app bot
Loading

Add support for the HTTP Range header to SimpleHTTPServer to solve #86809

@bedevere-app bedevere-app bot mentioned this pull request May 11, 2024
Open
@imba-tjd
Copy link
Contributor
imba-tjd commented May 23, 2024 •
edited
Loading

I didn't use it, but LGTM. Except I would prefer parse_range than get_range.

picnixz
picnixz reviewed May 23, 2024
View reviewed changes
@lyc8503 lyc8503 requested a review from picnixz May 23, 2024 14:30
picnixz
picnixz reviewed May 23, 2024
View reviewed changes
Copy link
Member
@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

(I've just reviewed at the same time you marked some comments as outdated)

@lyc8503 lyc8503 requested a review from picnixz May 23, 2024 15:36
picnixz
picnixz reviewed May 23, 2024
View reviewed changes
Copy link
Member
@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

A few comments (I won't be available afterwards but I may comment tomorrow)

@lyc8503
Copy link
Contributor Author
lyc8503 commented May 23, 2024

Thanks for the suggestions, I've made some changes based on them.

@lyc8503 lyc8503 requested a review from picnixz May 23, 2024 16:37
picnixz
picnixz reviewed May 24, 2024
View reviewed changes
Copy link
Member
@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Aaaand some final comments I think. Sorry to break down my reviews like that but I usually comment incrementally but I don't want people to be frustrated by my nitpicking (also, I don't see the point of reviewing something that could change after addressing other comments).

@lyc8503
Copy link
Contributor Author
lyc8503 commented May 24, 2024

Aaaand some final comments I think. Sorry to break down my reviews like that but I usually comment incrementally but I don't want people to be frustrated by my nitpicking (also, I don't see the point of reviewing something that could change after addressing other comments).

No worries, and thanks a lot for the detailed suggestions for my code! Your suggestions are very helpful.

@lyc8503 lyc8503 requested a review from picnixz May 24, 2024 07:40
picnixz
picnixz previously approved these changes May 24, 2024
View reviewed changes
Copy link
Member
@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I personally don't have more comments to add (but as always, it's easy to miss something obvious when you've reviewd the same thing multiple times).

@hondogitsune
Copy link
hondogitsune commented Jun 28, 2024 •
edited
Loading

@arhadthedev
@JelleZijlstra
This would close a nearly 4 year old issue and enhance cpython with one of the most basic features of HTTP servers, range requests.

Please review if possible.

@JCash
Copy link
JCash commented Dec 8, 2024

Ping! (I hope it's ok to ping every 6 months or so, especially if it seems to be so nearly done :) )

picnixz
picnixz reviewed Dec 8, 2024
View reviewed changes
Copy link
Member
@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Now that I've got more insights on how CPython's workflow works, I'd like you to address the following changes.

@picnixz picnixz dismissed their stale review December 8, 2024 23:37

Additional work is needed (work that I wasn't aware of in May...)

imba-tjd
imba-tjd reviewed Dec 10, 2024
View reviewed changes
@ghost
Copy link
ghost commented Dec 15, 2024 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@picnixz
Copy link
Member
picnixz commented Jan 3, 2025

We probably changed it historically though I'm not sure we are still changing it for every minor feature. Versioning is now delegated to git and Python versions themselve. We can increase the number but I wouldn't bother (check git blame to see when it was last updated for instance)

vadmium
vadmium reviewed Jan 3, 2025
View reviewed changes
@lyc8503
Copy link
Contributor Author
lyc8503 commented Jan 7, 2025

Does this need to be changed?
We can increase the number but I wouldn't bother (check git blame to see when it was last updated for instance)

This field has not been changed for 17 years😂. I guess we can leave it as is.

image

@lyc8503 lyc8503 requested review from picnixz and vadmium January 9, 2025 05:45
picnixz
picnixz reviewed Jan 11, 2025
View reviewed changes
Copy link
Member
@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

The coverage is good but the test function is a bit big. Maybe it's better to split into multiple functions (for instance, we can also name it test_single_range_get_* and have a test_multi_range_get case which only verifies that we ignore that part for now)

@lyc8503 lyc8503 requested a review from picnixz January 12, 2025 06:14
picnixz
picnixz reviewed
View reviewed changes
Copy link
Member
@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Almost there. The remaining are just nitpicks to ease future extensions and for tracking future issues if any.

@lyc8503
Copy link
Contributor Author
lyc8503 commented Jan 13, 2025

FTR, can we actually a link that that comment to indicate the rationale of this choice? Since the PR has quite a lot of comments, it'd be easier if we want to find the rationale in the future.

Which specific comment do you refer to?

BTW, I realized that parse_range() collapses (None, None) to None simply because bytes=- is not a valid range-specifier syntax, and I edited comments in code to indicate this.

@picnixz
Copy link
Member
picnixz commented Jan 13, 2025

Which specific comment do you refer to?

This one: #118949 (comment) or #118949 (comment).

picnixz
picnixz approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member
@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Sorry, I missed the What's New entry where using ~ would only show send_error and not BaseHTTPRequestHandler.send_error.

I'll let @vadmium decide when to merge after you've addressed that nit.

lyc8503 and others added 2 commits January 13, 2025 23:04
@lyc8503 @picnixz
Update Doc/whatsnew/3.14.rst
3033c27 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@lyc8503
Merge branch 'main' into pythongh-86809
e4b3607
@spacesynth
Copy link
spacesynth commented Feb 6, 2025 •
edited
Loading

@vadmium
Please forgive me for pinging, you are probably swamped in work! If you have time to look at this <200 LoC PR any time I'd be giggling in glee!

@lyc8503
Copy link
Contributor Author
lyc8503 commented May 9, 2025

Oh... I suddenly realized that Python 3.14 has already reached beta freeze, but this PR hasn't been merged yet, should I change the target version number to 3.15?

@picnixz
Copy link
Member
picnixz commented May 9, 2025

Yes, I'm sorry I forgot about it.

@lyc8503
Copy link
Contributor Author
lyc8503 commented May 9, 2025

No worries, I didn't notice that either. Let me change it tomorrow.

@picnixz picnixz self-requested a review May 26, 2025 23:22
@hondogitsune
Copy link
hondogitsune commented May 27, 2025 •
edited
Loading

Sorry for the thread bump:
What future Python version is being targeted at the moment for the release of thi 179B s good feature?

@lyc8503
Copy link
Contributor Author
lyc8503 commented May 28, 2025

What future Python version is being targeted at the moment for the release of this good feature?

Since we forgot to merge it in before the 3.14 feature freeze, you'll only see it in Python 3.15 at the earliest (a little over a year from now).

Let me change it tomorrow.

Sorry I haven't changed it yet. I see that there are still a lot of TODOs in the 3.15 whatsnew file, and I'm worried that there will be conflicts soon in the future if I change it now. Maybe I'll wait for the 3.15 whatsnew file to have a little bit more content.

@picnixz
Copy link
Member
picnixz commented May 30, 2025

Don't worry about the todos. Just add a new http (or http.client I don't remember where the code is) section under improved modules (it there isn't oe already).

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

@imba-tjd imba-tjd imba-tjd left review comments

@vadmium vadmium Awaiting requested review from vadmium

@picnixz picnixz Awaiting requested review from picnixz

Assignees
No one assigned
Labels
awaiting merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lyc8503 @imba-tjd @hondogitsune @JCash @picnixz @spacesynth @vadmium
0