Added Google's monkeypatch, disabled caching for now. #383

IntegersOfK · 2016-09-02T16:15:08Z

Added Google's monkeypatch so requests will use the appengine urlfetch library. Disabled caching for now because app engine does not work with the existing strategy.

This change is

…h library. Disabled caching for now because app engine does not work with the existing strategy.

IntegersOfK · 2016-09-02T16:16:15Z

Hi I actually have never made a pull request so forgive me if this isn't really what I'm supposed to do with it, I just wanted to illustrate how my app engine branch works. Take it or reject it as you will. Discussed in #44.

bear · 2016-09-02T16:29:09Z

Hi!

First, thanks for helping make the library better by fixing issues! That is amazing!

The failing checks you see above are from the PEP8 based lint check we have, that means that something in the code you added does not pass the PEP8 spec. You can see what they are by running locally make lint and then checking the contents of the file violations.flake8.txt which will have a line for each issue it noticed.

Once the make lint step passes then we can start looking at the change your wanting to do :)

IntegersOfK · 2016-09-02T17:50:43Z

Wow neat (and strict!) So now we're obviously going to have an import problem with the automated testing because it doesn't know where this crazy requests_toolbelt library is coming from. And also, what's the convention to explain these other things?

You should make sure SSL and requests are in your project's app.yaml
You need to put the requests_toolbelt library folder in the same place as app.yaml

Thanks for your warm wishes, I'm happy to dig more into this so if you can help me follow the proper processes along the way.

bear · 2016-09-02T18:22:58Z

I would start by adding a section to the README that says something like "See GAE.md for more details about how to use python-twitter with Google App Engine" -- that will let you go into detail about what is required without making the core README huge.

Another thing I would suggest is to not do the imports at the top of the file but instead inside of the api()'s init method so they are only handled if the caller is asking to use GAE -- right now your code fails for me because I'm not using GAE and don't have those modules in my system.

A good example of this is the failing tests:

______________________ ERROR collecting tests/test_api.py ______________________
ImportError while importing test module '/home/ubuntu/python-twitter/tests/test_api.py'.
Original error message:
'No module named requests_toolbelt.adapters.appengine'
Make sure your test modules/packages have valid Python names.

…and adapters to init function with an environment check.

…e App Engine.

bear · 2016-09-02T23:25:50Z

Reviewed 2 of 3 files at r3.
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

twitter/api.py, line 181 [r3] (raw file):

        # check to see if the library is running on a Google App Engine instance
        # see GAE.rst for more information
        if 'Google App Engine' in os.environ['SERVER_SOFTWARE']:

You will need to check for the existence of 'SERVER_SOFTWARE' in os.environ also - see the failing tests for the error report

Comments from Reviewable

IntegersOfK · 2016-09-03T07:41:37Z

I made a new virtualenv and everything to try to get the same errors as the tests, I don't know why these things aren't failing for me in the same way. Anyway, I'm still working on it, cool testing suite!

jeremylow · 2016-09-03T10:40:31Z

What's the command that you're using to run the test suite?

jeremylow · 2016-09-03T10:43:07Z

twitter/api.py

-				import requests_toolbelt.adapters.appengine  # Adapter ensures requests use app engine's urlfetch
-				requests_toolbelt.adapters.appengine.monkeypatch()
-				cache = None  # App Engine does not like this caching strategy, disable caching
+            if 'Google App Engine' in os.environ.get('SERVER_SOFTWARE', None):


os.environ.get will return a string, so you'll want to check via something like:

if os.environ.get('SERVER_SOFTWARE', None) == 'Google App Engine':

In this case the value for SERVER_SOFTWARE is actually different depending on the version. Ie. ' 'Google App Engine/3.25' or whatever it might be, so I can't check the whole string.

codecov-io · 2016-09-03T14:57:50Z

Current coverage is 69.00% (diff: 50.00%)

Merging #383 into master will decrease coverage by 0.05%

@@             master       #383   diff @@
==========================================
  Files             8          8          
  Lines          2014       2020     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1391       1394     +3   
- Misses          623        626     +3   
  Partials          0          0

Powered by Codecov. Last update 31f9df8...fd54732

IntegersOfK · 2016-09-03T15:00:43Z

I realise this has been a bit of a train wreck so far, but I've been learning a lot so thank you for following along.

bear · 2016-09-03T15:01:49Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

bear · 2016-09-03T15:02:09Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

bear · 2016-09-03T15:02:33Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

bear · 2016-09-03T15:04:19Z

it's both exciting and scary to submit a code change to someone-else's project so I'm glad you have and also glad you are letting us point the tweaks needed!

IntegersOfK · 2016-09-03T15:06:57Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

twitter/api.py, line 181 [r3] (raw file):

Previously, bear (Mike Taylor) wrote…

You will need to check for the existence of 'SERVER_SOFTWARE' in os.environ also - see the failing tests for the error report

Done.

twitter/api.py, line 182 [r4] (raw file):

Previously, IntegersOfK (AJ West) wrote…

In this case the value for SERVER_SOFTWARE is actually different depending on the version. Ie. ' 'Google App Engine/3.25' or whatever it might be, so I can't check the whole string.

Done.

Comments from Reviewable

bear · 2016-09-03T15:08:37Z

Once we let Jeremy look over the changes and he +1's them we can then merge \o/

jeremylow · 2016-09-03T15:28:39Z

Reviewed 2 of 3 files at r3, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

jeremylow · 2016-09-03T15:31:09Z

With the caveat that I'm not super familiar with Google App Engine, everything looks good to me! Thanks!

jeremylow · 2016-09-03T15:36:12Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

IntegersOfK · 2016-09-03T16:53:25Z

I should also mention that I am using this latest version on my Google App Engine project and it's working as expected.

bear · 2016-09-03T18:12:36Z

\o/

Thanks for the improvements!

jeremylow · 2016-10-03T01:31:00Z

Does anyone have any objection to me moving GAE.rst into the docs folder? It's not getting picked up on readthedocs.io if it's in the root dir :(

Added Google's monkeypatch so requests will use the appengine urlfetc…

4ad869d

…h library. Disabled caching for now because app engine does not work with the existing strategy.

IntegersOfK mentioned this pull request Sep 2, 2016

python-twitter isn't compatible with google app engine #44

Open

Corrected for PEP8 styling guide.

d88c4af

IntegersOfK added 3 commits September 2, 2016 16:40

Created a readme for Google App Engine, consolidated related imports …

d4678a8

…and adapters to init function with an environment check.

Update Google App Engine readme.

090e09c

Update the main README to direct readers to more information on Googl…

d9ce8a5

…e App Engine.

IntegersOfK added 6 commits September 3, 2016 02:43

Made check for os.environ existence, now graceful.

b822e63

Added additional check.

85e470a

Minor PEP8 fix.

293accc

Trying again to pass test, changed from None to boolean.

e29a414

Separated the checks.

f79ce60

Space/tab.

a68a404

jeremylow reviewed Sep 3, 2016
View reviewed changes

Trying again to solve this GAE check.

fd54732

bear merged commit aad1189 into bear:master Sep 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Google's monkeypatch, disabled caching for now. #383

Added Google's monkeypatch, disabled caching for now. #383

IntegersOfK commented Sep 2, 2016 •

edited by bear

Loading

IntegersOfK commented Sep 2, 2016 •

edited

Loading

bear commented Sep 2, 2016

IntegersOfK commented Sep 2, 2016

bear commented Sep 2, 2016 •

edited

Loading

bear commented Sep 2, 2016

IntegersOfK commented Sep 3, 2016

jeremylow commented Sep 3, 2016

jeremylow Sep 3, 2016

IntegersOfK Sep 3, 2016

codecov-io commented Sep 3, 2016 •

edited

Loading

IntegersOfK commented Sep 3, 2016

bear commented Sep 3, 2016

bear commented Sep 3, 2016

bear commented Sep 3, 2016

bear commented Sep 3, 2016

IntegersOfK commented Sep 3, 2016

bear commented Sep 3, 2016

jeremylow commented Sep 3, 2016

jeremylow commented Sep 3, 2016

jeremylow commented Sep 3, 2016

IntegersOfK commented Sep 3, 2016

bear commented Sep 3, 2016

jeremylow commented Oct 3, 2016

Added Google's monkeypatch, disabled caching for now. #383

Added Google's monkeypatch, disabled caching for now. #383

Conversation

IntegersOfK commented Sep 2, 2016 • edited by bear Loading

IntegersOfK commented Sep 2, 2016 • edited Loading

bear commented Sep 2, 2016

IntegersOfK commented Sep 2, 2016

bear commented Sep 2, 2016 • edited Loading

bear commented Sep 2, 2016

IntegersOfK commented Sep 3, 2016

jeremylow commented Sep 3, 2016

jeremylow Sep 3, 2016

Choose a reason for hiding this comment

IntegersOfK Sep 3, 2016

Choose a reason for hiding this comment

codecov-io commented Sep 3, 2016 • edited Loading

Current coverage is 69.00% (diff: 50.00%)

IntegersOfK commented Sep 3, 2016

bear commented Sep 3, 2016

bear commented Sep 3, 2016

bear commented Sep 3, 2016

bear commented Sep 3, 2016

IntegersOfK commented Sep 3, 2016

bear commented Sep 3, 2016

jeremylow commented Sep 3, 2016

jeremylow commented Sep 3, 2016

jeremylow commented Sep 3, 2016

IntegersOfK commented Sep 3, 2016

bear commented Sep 3, 2016

jeremylow commented Oct 3, 2016

IntegersOfK commented Sep 2, 2016 •

edited by bear

Loading

IntegersOfK commented Sep 2, 2016 •

edited

Loading

bear commented Sep 2, 2016 •

edited

Loading

codecov-io commented Sep 3, 2016 •

edited

Loading