fix: allow downloading when content-length header is missing#2413
Conversation
|
This makes the following work --8<-- "https://py.cafe/files/maartenbreddels/altair-car-performance-comparison/app.py"Which gives these headers: PS: this is used to render the sourcecode for the hosted app at https://py.cafe/maartenbreddels/altair-car-performance-comparison |
|
Seems like a pretty reasonable approach. We can ignore any lint errors outside of your changes (looks like some new checks are now throwing errors in old code that used to pass). Tests will have to be updated to pass for new expectations as before we used to always error out for missing content length. I imagine we will likely need at least one new test to cover this new use case as well. If this seems like a little beyond what you are comfortable doing (as it will probably involve doing some mocking and such), I can probably take what you have and finish, but feel free to update if you are comfortable in this area. Either way, I think this is a decent way to move forward on this. |
This can happen for valid http responses, such as when the server is using chunked encoding. Fixes facelessuser#2404
b3e9c99 to
03933ba
Compare
|
Hi, no problem with adding the test. Hope this is to your liking. Regards, Maarten |
pymdownx/snippets.py
Outdated
| content_length = int(length) | ||
| content = None | ||
| if "content-length" not in response.headers: | ||
| # we have to read to know if we went over the max, but never more than url_max_size |
There was a problem hiding this comment.
Please wrap url_max_size in backticks (`) so that the spellchecker will ignore them.
| for l in f: | ||
| length += len(l) | ||
| content.append(l) | ||
| content = open('tests/test_extensions/_snippets/lines.txt', 'rb').read() |
There was a problem hiding this comment.
While I'm indifferent as to whether open or with open is used, if with is not used, please explicitly close the file handle. with will take care of this for you. I understand that Cpython (specifically) will likely garbage collect this due to it not having a reference and going out of scope, but that is not the case with all Python implementations and I would like it to be explicitly closed, whether with with or without it.
|
Overall it looks good, but I'd made a few comments. |
|
Good comments 👍 . I implemented them as two separate commits to help review. Feel free to squash merge it when ready. |
|
@maartenbreddels Thanks for your work on this! @gir-bot lgtm |
|
Not to put pressure, but what is your release cadence? I want to advertise to people to use pycafe + mkdocs and this plugin to have live code examples in their docs, which depends on this fix. Could you let me know here or in #2404 when this is released, then I can let people know this is possible. Thanks! |
|
I imagine it will be sometime soon. I can't promise I'll remember to comment. The best way to know when a release is made is to watch the repo, and if all you care about is releases, configure it to only watch releases. You can disable the watch once the release in question is made. |
|
Really good idea, will do that! Thanks a ton, also, for these great extensions, and from what I've seen a very nice and high-quality codebase. Cheers |
This can happen for valid http responses, such as when the server is using chunked encoding.
Fixes #2404.