RFC 7233 - Range Requests #977

magne4000 · 2016-08-03T07:51:49Z

See #978 for details.

magne4000 · 2016-08-05T13:48:03Z

Travis seems to have a hard time running redis server on last build :)

homu · 2016-08-12T13:48:23Z

☔ The latest upstream changes (presumably ca1665a) made this pull request unmergeable. Please resolve the merge conflicts.

homu · 2016-08-14T21:58:10Z

☔ The latest upstream changes (presumably 2bc6035) made this pull request unmergeable. Please resolve the merge conflicts.

tests/test_wrappers.py

@@ -538,6 +540,69 @@ def test_etag_response_mixin():
    response.make_conditional(env)
    assert response.content_length == 999

+    # simple valid Range Request


untitaker · 2016-08-19T15:27:17Z

@magne4000 I think you goofed up the diff, it now contains unrelated changes.

I recommend git rebase -i <base_branch> to select which commits should stay.

magne4000 · 2016-08-20T09:07:33Z

I had to soft reset because I messed up the history, but it's cleaner now.

homu · 2016-08-22T23:51:30Z

☔ The latest upstream changes (presumably 0407d22) made this pull request unmergeable. Please resolve the merge conflicts.

werkzeug/http.py

            end = None
            last_end = -1
        elif '-' in item:
            begin, end = item.split('-', 1)
+            begin = begin.strip()
+            end = end.strip()
+            if not begin.isdigit():


untitaker · 2016-08-23T00:35:18Z

Excellent work, it's really rare to get a PR of this size with so few errors. Sorry it took so long to review this, but here we go.

werkzeug/wrappers.py

+                self.status_code != 206
+                and not is_resource_modified(
+                    environ, self.headers.get('etag'),
+                    None, self.headers.get('last-modified')


untitaker · 2016-09-07T09:33:06Z

There's one issue left: There are several new public functions. It needs to be decided which ones we want to expose, those have to be added to the .rst files in the docs folder.

magne4000 · 2016-09-07T11:17:56Z

werkzeug/datastructures.py new method Range.to_content_range_header already handled by:

.. autoclass:: Range
   :members:

werkzeug/wrappers.py new methods are considered protected
werkzeug/__init__.py add 'RangeWrapper' to 'werkzeug.wsgi'
werkzeug/wsgi.py should add RangeWrapper in wsgi.rst

Don't know if we need something more

untitaker · 2016-09-07T12:34:25Z

If they're protected they should be prefixed by underscore IMO, but will have to look how rest of Wz handles this.

On 7 September 2016 13:17:58 CEST, "Joël Charles" notifications@github.com wrote:

werkzeug/datastructures.py new method
Range.to_content_range_header already handled by:
.. autoclass:: Range
  :members:
werkzeug/wrappers.py new methods are considered protected

werkzeug/__init__.py add 'RangeWrapper' to 'werkzeug.wsgi'

werkzeug/wsgi.py should add RangeWrapper in wsgi.rst

Don't know if we need something more

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#977 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

magne4000 · 2016-09-07T12:36:02Z

That is already the case

untitaker · 2016-09-07T12:43:48Z

Ah sorry, I forgot there were mostly new methods there. Do we really want to expose RangeWrapper? It seems like out of scope for Werkzeug as a WSGI library (like many things in Werkzeug, see #4). Also imports from __init__ will be deprecated eventually.

untitaker · 2016-09-08T09:11:08Z

Also not our problem, but I'd rather create a third-party package with this API than exposing it here.

On 8 September 2016 11:08:10 CEST, Armin Ronacher notifications@github.com wrote:

Not our problem tbh

Then we should quickly figure out the answers to these two questions:

Is there a value for a user to create instances of this object?

If the answer to 1. is yes, are they supposed to copy paste it?

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#977 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

mitsuhiko · 2016-09-08T09:12:48Z

@untitaker sure. But we already need to include this thing internally anyways. So it's not like we can delete that code.

magne4000 · 2016-09-08T09:16:32Z

We shouldn't incitate one to instanciate this class. Instead, as I added in the doc, the response data object must implement a part of io.BaseIO methods.

untitaker · 2016-09-08T09:17:18Z

If we suddenly don't need it anymore we can't delete it either without API breakage. With your philosophy taken to the extreme, no refactor would be possible without bumping the major version.

On 8 September 2016 11:12:51 CEST, Armin Ronacher notifications@github.com wrote:

@untitaker sure. But we already need to include this thing internally
anyways. So it's not like we can delete that code.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#977 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

mitsuhiko · 2016-09-08T09:20:06Z

Let's keep it private for now then and add a comment to decide on what to do with it in the future.

untitaker · 2016-09-08T09:59:37Z

@magne4000 Could you add that comment? Thanks!

magne4000 · 2016-09-08T11:48:51Z

Should I prefix the comment with a specific sphinx markup ?

…nnal to give details on how to optimize it's use in Range Request context

untitaker · 2016-09-08T11:55:44Z

No I think a normal #-comment is fine.

On Thu, Sep 08, 2016 at 04:48:53AM -0700, Joël Charles wrote:

Should I prefix the comment with a specific sphinx markup ?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#977 (comment)

untitaker · 2016-09-08T13:34:23Z

Excellent, thank you!

magne4000 · 2016-09-08T13:47:40Z

😉

untitaker · 2016-09-08T13:49:18Z

Also thanks for being patient with the bit-by-bit review I did. I do try to review your PR in large chunks, but some things only come to mind after a while.

magne4000 · 2016-09-08T13:55:32Z

No probs, that was necessary, this was not a tiny PR 😄

davidism · 2016-09-08T14:31:22Z

Impressive work, thanks!

magne4000 changed the title ~~Fix malformed Range header parsing. Closes #974~~ RFC 7233 - Range Requests Aug 5, 2016

magne4000 mentioned this pull request Aug 10, 2016

Unable to seek ahead of what has already been loaded magne4000/festival#44

Closed

untitaker reviewed Aug 14, 2016
View reviewed changes

tests/test_wrappers.py

@@ -538,6 +540,69 @@ def test_etag_response_mixin():

response.make_conditional(env)

assert response.content_length == 999

# simple valid Range Request

This comment was marked as off-topic.

Sign in to view

magne4000 force-pushed the master branch from 5b5fb8a to ba28aaa Compare August 20, 2016 08:58

untitaker reviewed Aug 22, 2016
View reviewed changes

werkzeug/http.py

end = None

last_end = -1

elif '-' in item:

begin, end = item.split('-', 1)

begin = begin.strip()

end = end.strip()

if not begin.isdigit():

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

magne4000 force-pushed the master branch 2 times, most recently from 2837b2f to 3f61999 Compare August 30, 2016 07:12

untitaker reviewed Sep 6, 2016
View reviewed changes

magne4000 added 8 commits September 8, 2016 13:52

Implements RFC 7233 - Range Requests

ebe7900

Ignore If-Range header if Range header is not present

6090536

Wrappers cleanup regarding Range Request handling

53922b1

Less hacky RangeWrapper logic

2b0ec29

Fix len in test case

0c60033

Range and If-Range are not response headers ...

492bf52

Rename RangeWrapper to _RangeWrapper, and update doc of make_conditio…

d943e76

…nnal to give details on how to optimize it's use in Range Request context

Do not be to verbose in make_conditionnal doc

e27fde6

magne4000 force-pushed the master branch from 2a02bde to e27fde6 Compare September 8, 2016 11:53

Added a comment about _RangeWrapper future

eabca82

untitaker merged commit 0a23c94 into pallets:master Sep 8, 2016

untitaker mentioned this pull request Sep 8, 2016

RFC 7233 - Range Requests #978

Closed

hhm0 mentioned this pull request Mar 19, 2017

use range requests when available spl0k/supysonic#57

Merged

davidism mentioned this pull request Dec 3, 2018

Malformed Range header causes ETagRequestMixin.range to fail #974

Closed

davidism mentioned this pull request Feb 6, 2020

Can't serve html video #410

Closed

chrahunt mentioned this pull request Jul 13, 2020

Use lazy wheel to obtain dep info for new resolver pypa/pip#8532

Closed

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC 7233 - Range Requests #977

RFC 7233 - Range Requests #977

magne4000 commented Aug 3, 2016 •

edited

Loading

magne4000 commented Aug 5, 2016

homu commented Aug 12, 2016

homu commented Aug 14, 2016

This comment was marked as off-topic.

untitaker commented Aug 19, 2016

magne4000 commented Aug 20, 2016

homu commented Aug 22, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

untitaker commented Aug 23, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

untitaker commented Sep 7, 2016

magne4000 commented Sep 7, 2016

untitaker commented Sep 7, 2016

magne4000 commented Sep 7, 2016

untitaker commented Sep 7, 2016

untitaker commented Sep 8, 2016

mitsuhiko commented Sep 8, 2016

magne4000 commented Sep 8, 2016

untitaker commented Sep 8, 2016

mitsuhiko commented Sep 8, 2016

untitaker commented Sep 8, 2016

magne4000 commented Sep 8, 2016

untitaker commented Sep 8, 2016

untitaker commented Sep 8, 2016

magne4000 commented Sep 8, 2016

untitaker commented Sep 8, 2016

magne4000 commented Sep 8, 2016

davidism commented Sep 8, 2016

RFC 7233 - Range Requests #977

RFC 7233 - Range Requests #977

Conversation

magne4000 commented Aug 3, 2016 • edited Loading

magne4000 commented Aug 5, 2016

homu commented Aug 12, 2016

homu commented Aug 14, 2016

This comment was marked as off-topic.

untitaker commented Aug 19, 2016

magne4000 commented Aug 20, 2016

homu commented Aug 22, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

untitaker commented Aug 23, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

untitaker commented Sep 7, 2016

magne4000 commented Sep 7, 2016

untitaker commented Sep 7, 2016

magne4000 commented Sep 7, 2016

untitaker commented Sep 7, 2016

untitaker commented Sep 8, 2016

mitsuhiko commented Sep 8, 2016

magne4000 commented Sep 8, 2016

untitaker commented Sep 8, 2016

mitsuhiko commented Sep 8, 2016

untitaker commented Sep 8, 2016

magne4000 commented Sep 8, 2016

untitaker commented Sep 8, 2016

untitaker commented Sep 8, 2016

magne4000 commented Sep 8, 2016

untitaker commented Sep 8, 2016

magne4000 commented Sep 8, 2016

davidism commented Sep 8, 2016

magne4000 commented Aug 3, 2016 •

edited

Loading