Skip to content

Add heroku button #4

Merged
merged 18 commits into from Aug 22, 2014

3 participants

@friism
friism commented Aug 20, 2014

Supersedes PR #2

@zheller zheller commented on an outdated diff Aug 21, 2014
@@ -1,6 +1,8 @@
from __future__ import absolute_import
from flask import Flask, render_template, request, redirect, session
from rauth import OAuth2Service
+from urlparse import urlparse
+from flask_sslify import SSLify
@zheller
Uber member
zheller added a note Aug 21, 2014

please alphabetize imports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@zheller zheller and 1 other commented on an outdated diff Aug 21, 2014
@@ -1,6 +1,8 @@
from __future__ import absolute_import
from flask import Flask, render_template, request, redirect, session
from rauth import OAuth2Service
+from urlparse import urlparse
@zheller
Uber member
zheller added a note Aug 21, 2014

standard lib imports should be on their own block above

@friism
friism added a note Aug 22, 2014

I'm guessing the block below (with json and friends)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@zheller zheller commented on the diff Aug 22, 2014
@@ -214,6 +217,11 @@ def me():
data=response.text,
)
+def get_redirect_uri(request):
@zheller
Uber member
zheller added a note Aug 22, 2014

docstring please

@friism
friism added a note Aug 22, 2014

Added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@zheller zheller and 1 other commented on an outdated diff Aug 22, 2014
@@ -214,6 +217,11 @@ def me():
data=response.text,
)
+def get_redirect_uri(request):
+ parsed_url = urlparse(request.url)
+ if parsed_url.hostname == 'localhost':
+ return str.format('http://{0}:{1}/submit', parsed_url.hostname, parsed_url.port)
@zheller
Uber member
zheller added a note Aug 22, 2014

Our convention is 'my string {variable_name}'.format(variable_name='is awesome')

@friism
friism added a note Aug 22, 2014

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@friism
friism commented Aug 22, 2014

@zheller Thanks for the feedback! Let me know if I missed anything

@zheller
Uber member
zheller commented Aug 22, 2014

Cool, i think you may need to rebase and fix a merge conflict but its a +1 from me!

@kaushal

Instead of "both OAuth scopes" we should specify that the profile and history scopes are needed specifically.

@friism
friism commented Aug 22, 2014

@kaushal @zheller changed the scope wording and it's rebased on master

@zheller zheller commented on an outdated diff Aug 22, 2014
import json
+import os
+import requests
@zheller
Uber member
zheller added a note Aug 22, 2014

oops forgot this, requests is not a std lib, you should also swap this section and above -- i think this was a problem in our code initially but would love if you did us a solid here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@friism
friism commented Aug 22, 2014

@zheller like so? (sorry about this back-and-forth, I don't write Python in my dayjob)

@zheller
Uber member
zheller commented Aug 22, 2014

looks great! Thanks @friism

@zheller zheller merged commit 0c6989e into uber:master Aug 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.