GoogleCloudPlatform / webapp2

webapp2 is a framework for Google App Engine
https://webapp2.readthedocs.org
Other
141 stars 63 forks source link

Added support for python 2 and 3 #117

Closed mymtw closed 8 years ago

mymtw commented 8 years ago

I have added support for python 2.7+ and python 3.3+ versions. All of the tests for python 2.7+ were successfully passed. But when I tried to run tests on python 3, I got some troubles: one of them it was installing gae sdk

Maybe we need to add separated tests for webapp2, which will be run on python 3, without webapp2_extras? Any ideas?

theacodes commented 8 years ago

It looks like we need to cleanly separate the app engine-specific bits and not run tests against those on py3. Do you want to take a stab at that? Let me know if you need help.

mymtw commented 8 years ago

Do you want to take a stab at that?

yes, I do

Let me know if you need help.

ok

mymtw commented 8 years ago

@jonparrott I think that I practically completed with this, but: 1) Can you help with travis config, that he runned this command tests_outside_gaesdk on py2.7 and py3.5 2) I got error when tried to run nox -s test, gae said ImportError: No module named yaml(even in upstream/master same error) I added in conftest.py next row dev_appserver.fix_sys_path() and it was helped. Is it ok? 3) The tests folder got new structure, should we change it? need your opinion about:

tests/
├── resources
│   ├── handlers.py
│   ├── i18n.py
│   ├── __init__.py
│   ├── jinja2_templates
│   │   ├── hello.html
│   │   ├── template1.html
│   │   ├── template2.html
│   │   └── template3.html
│   ├── jinja2_templates_compiled
│   │   └── tmpl_3a79873b1b49be244fd5444b1258ce348be26de8.py
│   ├── mako_templates
│   │   └── template1.html
│   ├── protorpc_services.py
│   └── template.py
├── gae
│   ├── conftest.py
│   ├── extras_appengine_auth_models_test.py
│   ├── extras_appengine_sessions_memcache_test.py
│   ├── extras_appengine_sessions_ndb_test.py
│   ├── extras_appengine_users_test.py
│   ├── extras_auth_test.py
│   ├── __init__.py
│   ├── test_base.py
│   └── webapp1_test.py
├── extras_config_test.py
├── extras_i18n_test.py
├── extras_jinja2_test.py
├── extras_json_test.py
├── extras_local_app_test.py
├── extras_mako_test.py
├── extras_routes_test.py
├── extras_securecookie_test.py
├── extras_security_test.py
├── extras_sessions_test.py
├── extras_xsrf_test.py
├── handler_test.py
├── __init__.py
├── misc_test.py
├── request_test.py
├── response_test.py
├── routing_test.py
└── test_base.py
theacodes commented 8 years ago

New organization looks fine to me. I'm a bit busy this week, but I should be able to review and fix the yaml and travis issues.

It'll help If you give me access to your fork to push commits to your branch so that we can do this together in one PR.

Thanks so much for doing this!

mymtw commented 8 years ago

I've just sent invite

theacodes commented 8 years ago

Finished my first review pass, I still haven't had a chance to tinker with tests.

My biggest concern is the dependency on six. I'm fine with us depending on it, but I want to make sure that we're depending on it because we need it.

mymtw commented 8 years ago

My biggest concern is the dependency on six. I'm fine with us depending on it, but I want to make sure that we're depending on it because we need it.

if you meant

1) I decided that it's the best way to use already ready tool for support 2/3 code with help of which, we will no need to write redundancy code everywhere, like below and maybe w/o of fear for some new changes in python 3.x imports.

import sys
PY2 = sys.version_info[0] == 2
PY3 = sys.version_info[0] == 3
if PY2:
   ....
if PY3:
   ....

if we can write more cleaner:

import six
if six.PY2:
  ....
if six.PY3:
   ....

or yet again trash:

try:
  from urllib import quote
except ImportError:
 from urllib.parse import quote

if we can write more cleaner:

import six
from six.moves.urllib.parse import quote

range, xrange Here last example, we had a lot of cases like if isinstance(query, basestring) And we should change it on:

try:
  basestring
except NameError:
  basestring = str

if isinstance(query, basestring):

instead of

import six
if isinstance(query, six.string_types):

2) yes webOb and flask added own compat module: https://github.com/pallets/flask/blob/master/flask/_compat.py https://github.com/Pylons/webob/blob/master/webob/compat.py django.utils.six... But create a new file in project instead of simple installing of it: idk ¯_(ツ)_/¯ I didn't see reason, only if extra dependencies - problem... Also about dependencies, six haves(pytest, flake8) We can create compat module, if you want, no problem

theacodes commented 8 years ago

Alright, I'm okay with six. Let's go with that.

codecov-io commented 8 years ago

Current coverage is 94.87% (diff: 87.20%)

Merging #117 into master will decrease coverage by 0.05%

@@             master       #117   diff @@
==========================================
  Files            23         23          
  Lines          1894       1914    +20   
  Methods           0          0          
  Messages          0          0          
  Branches        286        291     +5   
==========================================
+ Hits           1798       1816    +18   
+ Misses           38         37     -1   
- Partials         58         61     +3   

Powered by Codecov. Last update 0900b5a...cd190a8

theacodes commented 8 years ago

@mymtwcom I pushed a commit to your branch that fixes up nox and conftests (yay!). There seems to be a few tests failures - if you can get those resolved then this looks good.

mymtw commented 8 years ago

@jonparrott I added fixes for these tests. There are were py.test problem, which I didn't see cuz, I ran tests with --capture=no(I added mocking for stdin.buffers), and py3.4 compatibility bug. Currently, code coverage is 51.67%, but it was ~95%. I don't know why do this happens exactly, I didn't work with codecov. also plz check setup.py EXTRA_REQUIREMENTS var. Is it ok? And Travis: The path python3.5 (from --python=python3.5) does not exist

theacodes commented 8 years ago

@mymtwcom thanks! I'll look into all of this soon (sorry, busy week for me)

mymtw commented 8 years ago

kk

theacodes commented 8 years ago

@mymtwcom just an update that this is still on my radar! Apologies for the long turnaround.

theacodes commented 8 years ago

@mymtwcom I moved six from extras to install_requires, I also lowered the minimum version required for pytz to the oldest version that supports py3.5.

Pending travis, I can merge this.

mymtw commented 8 years ago

@jonparrott thanks for help

theacodes commented 8 years ago

@mymtwcom no seriously, thank you. This is a huge contribution!

theacodes commented 8 years ago

@mymtwcom once #107 is done we can cut a new release.