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.

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

fallback for platform who don't have SpooledTemporaryFile #1220

Closed
wants to merge 1 commit into from

Conversation

erlichmen
Copy link

@erlichmen erlichmen commented Dec 27, 2017

On AppEngine SpooledTemporaryFile is not supported, this PR will fallback to the old code when SpooledTemporaryFile is not supported on the platform (As discussed on #1210)

@awonak
Copy link

awonak commented Dec 27, 2017

Confirmed this fixes my particular case in GoogleCloudPlatform/python-docs-samples#1266

$ dev_appserver.py .                                           
INFO     2017-12-27 15:50:16,680 devappserver2.py:105] Skipping SDK update check.
INFO     2017-12-27 15:50:16,710 api_server.py:300] Starting API server at: http://localhost:37081
INFO     2017-12-27 15:50:16,742 dispatcher.py:251] Starting module "default" running at: http://localhost:8080
INFO     2017-12-27 15:50:16,742 admin_server.py:116] Starting admin server at: http://localhost:8000
ERROR    2017-12-27 15:50:19,038 wsgi.py:263] 
Traceback (most recent call last):
...
  File ".../test/lib/werkzeug/formparser.py", line 14, in <module>
    from tempfile import SpooledTemporaryFile
ImportError: cannot import name SpooledTemporaryFile
INFO     2017-12-27 15:50:19,045 module.py:821] default: "GET / HTTP/1.1" 500 -
$ pip install --upgrade -t lib git+https://github.com/erlichmen/werkzeug.git 
Collecting git+https://github.com/erlichmen/werkzeug.git
  Cloning https://github.com/erlichmen/werkzeug.git to /tmp/pip-cggmbu-build
Installing collected packages: Werkzeug
  Running setup.py install for Werkzeug ... done
Successfully installed Werkzeug-0.14.dev0
$ dev_appserver.py .                                                         
INFO     2017-12-27 15:51:01,358 devappserver2.py:105] Skipping SDK update check.
INFO     2017-12-27 15:51:01,390 api_server.py:300] Starting API server at: http://localhost:44439
INFO     2017-12-27 15:51:01,424 dispatcher.py:251] Starting module "default" running at: http://localhost:8080
INFO     2017-12-27 15:51:01,425 admin_server.py:116] Starting admin server at: http://localhost:8000
INFO     2017-12-27 15:51:04,695 module.py:821] default: "GET / HTTP/1.1" 200 2

@mitsuhiko
Copy link
Contributor

Closed in favor of #1222 which correctly handles chunked requests.

@mitsuhiko mitsuhiko closed this Dec 27, 2017
@erlichmen
Copy link
Author

@mitsuhiko I don't understand why open a new PR which has pretty much the same code instead of commenting on this PR.

@mitsuhiko
Copy link
Contributor

Not sure what would have been the value of commenting on the PR and waiting for feedback instead of just fixing the bug.

@erlichmen
Copy link
Author

@mitsuhiko I would image that the open source movement will look totally different if every external contribution PR will be just taken for source value and pasted by the maintainer instead of guiding.

And on a different note: I think this PR need to enter into a 0.13.1 state, currently every flask AppEngine Standard is broken upon updating the packages and freezing flask won't help. Everyone is going to discover this issue the hard way (read in production).

@mitsuhiko
Copy link
Contributor

I'm sorry I really don't understand what I did wrong here. There won't be a 0.13 but I will push out a 0.14 today or tomorrow most likely.

@ThiefMaster
Copy link
Member

IMO fixing a small thing you notice in a PR is much faster than pinging the original author of a PR - especially if you already know the fix. When having the PR's author fix it, you either need to mention exactly how it should be fixed (and then it's faster to just write it yourself) or you just mention the problem with the current code (then it may take much longer in the end for the PR to be correct).

The only thing I would have done differently is adding another commit to the PR's branch in @erlichmen's fork (I believe "allow maintainers to push" is enabled by default for PRs nowadays) instead of creating a completely new PR. Like this the original credit of the change remains. FWIW, for a small change like this I'd probably just amend the original commit and not even bother adding another commit, but I think that's mostly a matter of opinion (I guess some people might not like their commits being amended by others).

@mitsuhiko
Copy link
Contributor

The only thing I would have done differently is adding another commit to the PR's branch

I did not even know that is a thing.

@erlichmen
Copy link
Author

erlichmen commented Dec 28, 2017 via email

@davidism davidism added this to the 0.14 milestone Jan 3, 2018
@davidism
Copy link
Member

davidism commented Jan 3, 2018

@mitsuhiko for future reference, you can either push to PR branches now, or if you needed to create a separate PR you can still base it off the commits from this one.

@erlichmen sorry for the confusion, we still really appreciate your contribution! 👍

@@ -11,7 +11,14 @@
"""
import re
import codecs
from tempfile import SpooledTemporaryFile
try:
from tempfile import SpooledTemporaryFile

This comment was marked as off-topic.

This comment was marked as off-topic.

@erlichmen
Copy link
Author

erlichmen commented Mar 12, 2018 via email

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants