Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Importing top-level webapp2 breaks encapsulation #302

Closed
tseaver opened this issue Sep 8, 2015 · 5 comments
Closed

Importing top-level webapp2 breaks encapsulation #302

tseaver opened this issue Sep 8, 2015 · 5 comments

Comments

@tseaver
Copy link

tseaver commented Sep 8, 2015

f3f8b4f replaces an import of webapp from google.appengine.ext with a top-level import of webapp2 (aliased as webapp). The commit comment says:

This choice is apparently more robust in situations / runtimes where the import mechanism in google/appengine/ext/webapp/init.py does not appear to do the right thing.

This change breaks the GAE import mocking done in gcloud.

@dhermes
Copy link
Contributor

dhermes commented Sep 8, 2015

@tseaver How do you mean it breaks encapsulation? The oauth2client.appengine module isn't imported in the package, so a user needs to explicitly say "import me some appengine, I'm on the appengine".

@tseaver
Copy link
Author

tseaver commented Sep 8, 2015

The gcloud tests covering the "appengine detection" machinery mock out the necessary google.appengine bits to allow oauth2client.appengine to import without breaking. f3f8b4f breaks that, switching to import webapp as an alias for an entirely new top-level webapp2 package (newly-imported here, at least).

I don't know what the motivation for f3f8b4f was, but it caused the gcloud tests to break today due to the ues of newly-released oauth2client 1.5.0.

@dhermes
Copy link
Contributor

dhermes commented Sep 8, 2015

Motivation: webapp2 replaced webapp in either 2011 or 2010, so the code was very old.

I realize it hurt our test mocks in a library elsewhere, but that isn't an issue with oauth2client.

I'm closing out, let me know if you think there is something left in oauth2client that needs to be resolved.

@dhermes dhermes closed this as completed Sep 8, 2015
@tseaver
Copy link
Author

tseaver commented Sep 8, 2015

FTR, the commit was a fix for #217. That change added an import dependency on webapp2, without also adding as an install_requires dependency in setup.py: AFAICT, it should be there (d2c15e6 patched it into tox.ini for the docs). Perhaps it should be in an 'extras_require`, e.g.:

setup(
#...
    extras_require={'appengine': ['webapp2', 'appengine']},
)

@dhermes
Copy link
Contributor

dhermes commented Sep 8, 2015

App Engine is a nasty beast. Users deploy code there and all of these dependencies are just there. (webapp2 is luckily open source and available on PyPI)

tseaver referenced this issue Sep 9, 2015
This choice is apparently more robust in situations / runtimes where the import mechanism in google/appengine/ext/webapp/__init__.py does not appear to do the right thing.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants