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

Convert make_test_environ_builder into class (fixes #3207) #3232

Merged
merged 3 commits into from May 31, 2019

Conversation

@lordmauve
Copy link
Contributor

commented May 31, 2019

Added a new class flask.testing.EnvironBuilder inheriting from werkzeug.test.EnvironBuilder.

Logic from make_test_environ_builder() moved to the constructor of that class, and changed to simply instantiate the class, while issuing a DeprecationWarning.

I did explore making json_dumps() a regular method rather than a static method, to pick up app, but if anything was expecting to call EnvironBuilder.json_dumps() as a static method then this would break. Requires funky descriptor tricks to work both as a static method and an instance method under Python 2 and so didn't seem worth the code it would take.

closes #3207

@lordmauve

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Tests and stylecheck passed under Tox but somehow the Azure pipelines have not been requested for this PR. (I appear to be on a dodgy internet connection.)


if subdomain:
http_host = "{0}.{1}".format(subdomain, http_host)
if "json" in kwargs:

This comment has been minimized.

Copy link
@davidism

davidism May 31, 2019

Member

Should be able to remove this part, since the base EnvironBuilder handles JSON.

This comment has been minimized.

Copy link
@lordmauve

lordmauve May 31, 2019

Author Contributor

No, because the base class doesn't know about app and therefore does not take JSON serialisation settings from it.

This could be removed if json_dumps() was added as a method that respects self.app, but this is hard if json_dumps() needs to be callable as a static method as described above. If that's not needed, then yes, I can remove this.

This comment has been minimized.

Copy link
@lordmauve

lordmauve May 31, 2019

Author Contributor

In fact, I'm pretty sure it is a safe assumption that it's not needed. Let me go ahead and do this.

This comment has been minimized.

Copy link
@davidism

davidism May 31, 2019

Member

Yeah, that should work.

@davidism

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Looks good! Please add a changelog entry as well.

flask/testing.py Outdated Show resolved Hide resolved

@lordmauve lordmauve force-pushed the lordmauve:environbuilder branch from 036493e to e45370b May 31, 2019

@davidism davidism merged commit 5e15850 into pallets:master May 31, 2019

13 checks passed

Flask Build #20190531.18 succeeded
Details
Flask (Flask DocsHtml) Flask DocsHtml succeeded
Details
Flask (Flask Pypy3Linux) Flask Pypy3Linux succeeded
Details
Flask (Flask Python27Linux) Flask Python27Linux succeeded
Details
Flask (Flask Python27Windows) Flask Python27Windows succeeded
Details
Flask (Flask Python35Linux) Flask Python35Linux succeeded
Details
Flask (Flask Python36Linux) Flask Python36Linux succeeded
Details
Flask (Flask Python37Linux) Flask Python37Linux succeeded
Details
Flask (Flask Python37Mac) Flask Python37Mac succeeded
Details
Flask (Flask Python37Windows) Flask Python37Windows succeeded
Details
Flask (Flask Style) Flask Style succeeded
Details
Flask (Flask VersionRange) Flask VersionRange succeeded
Details
Flask (FlaskOnNightly) FlaskOnNightly succeeded
Details

@davidism davidism added this to the 1.1.0 milestone May 31, 2019

@lordmauve lordmauve deleted the lordmauve:environbuilder branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.