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

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

Merged
merged 12 commits into from Sep 3, 2016

Conversation

Projects
None yet
4 participants
@IntegersOfK
Contributor

IntegersOfK commented Sep 2, 2016

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 Reviewable

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

This comment has been minimized.

Contributor

IntegersOfK commented Sep 2, 2016

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

This comment has been minimized.

Owner

bear commented Sep 2, 2016

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

This comment has been minimized.

Contributor

IntegersOfK commented Sep 2, 2016

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?

  1. You should make sure SSL and requests are in your project's app.yaml
  2. 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

This comment has been minimized.

Owner

bear commented Sep 2, 2016

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.

IntegersOfK added some commits Sep 2, 2016

Created a readme for Google App Engine, consolidated related imports …
…and adapters to init function with an environment check.
@bear

This comment has been minimized.

Owner

bear commented Sep 2, 2016

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 added some commits Sep 3, 2016

@IntegersOfK

This comment has been minimized.

Contributor

IntegersOfK commented Sep 3, 2016

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

This comment has been minimized.

Collaborator

jeremylow commented Sep 3, 2016

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

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):

This comment has been minimized.

@jeremylow

jeremylow Sep 3, 2016

Collaborator

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':

This comment has been minimized.

@IntegersOfK

IntegersOfK Sep 3, 2016

Contributor

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

This comment has been minimized.

codecov-io commented Sep 3, 2016

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

This comment has been minimized.

Contributor

IntegersOfK commented Sep 3, 2016

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

This comment has been minimized.

Owner

bear commented Sep 3, 2016

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

This comment has been minimized.

Owner

bear commented Sep 3, 2016

:lgtm:


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


Comments from Reviewable

@bear

This comment has been minimized.

Owner

bear commented Sep 3, 2016

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

This comment has been minimized.

Owner

bear commented Sep 3, 2016

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

This comment has been minimized.

Contributor

IntegersOfK commented Sep 3, 2016

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

This comment has been minimized.

Owner

bear commented Sep 3, 2016

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

@jeremylow

This comment has been minimized.

Collaborator

jeremylow commented Sep 3, 2016

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

This comment has been minimized.

Collaborator

jeremylow commented Sep 3, 2016

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

@jeremylow

This comment has been minimized.

Collaborator

jeremylow commented Sep 3, 2016

:lgtm:


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


Comments from Reviewable

@IntegersOfK

This comment has been minimized.

Contributor

IntegersOfK commented Sep 3, 2016

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

@bear

This comment has been minimized.

Owner

bear commented Sep 3, 2016

\o/

Thanks for the improvements!

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

1 of 4 checks passed

codecov/patch 50.00% of diff hit (target 69.06%)
Details
codecov/project 69.00% (-0.06%) compared to 31f9df8
Details
code-review/reviewable 2 discussions left (bear, jeremylow)
Details
ci/circleci Your tests passed on CircleCI!
Details
@jeremylow

This comment has been minimized.

Collaborator

jeremylow commented Oct 3, 2016

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 :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment