x1ddos / simpleauth

Simple authentication for Python on Google App Engine supporting OAuth 2.0, OAuth 1.0(a) and OpenID
https://simpleauth.appspot.com
327 stars 61 forks source link

Cleanup and support for destination_url parameter #32

Closed okigan closed 10 years ago

erichiggins commented 10 years ago

First pass complete.

x1ddos commented 10 years ago

@okigan the tests are failing though: https://drone.io/github.com/okigan/simpleauth/latest

x1ddos commented 10 years ago

@erichiggins thanks so much for reviewing!

okigan commented 10 years ago

oh boy, how do I setup gae environment in IntelliJ 13?

okigan commented 10 years ago

Bump

okigan commented 10 years ago

@crhym3 @erichiggins pull request ready for merge, please proceed

erichiggins commented 10 years ago

@okigan It looks like there are still a few outstanding comments in the PR that you may need to address before merging.

@crhym3 Are you going to take over the review from here?

okigan commented 10 years ago

@erichiggins i don't see any required to fix, only comment/suggestions for future.

x1ddos commented 10 years ago

@erichiggins I don't want to take over, but I guess I could in case you don't have cycles.

PS sorry for slow responses. It's been crazy these days.

erichiggins commented 10 years ago

Sorry, I just realized I was reviewing with the wrong browser window/account.

erichiggins commented 10 years ago

@okigan That's about all of the review comments I have for now. Once you address those, I'll take another pass.

okigan commented 10 years ago

@erichiggins ok, go for "another pass"

erichiggins commented 10 years ago

This seems fine to me by @crhym3 should give the final review/approval.

okigan commented 10 years ago

@crhym3 bump

x1ddos commented 10 years ago

@okigan sorry, I was on a short vacation. Will be back tomorrow or Wed, but I've already pulled your changes. Just wanted to make sure all tests pass.

okigan commented 10 years ago

@crhym3 no problem (in fact, get some rest!), just wanted to make sure we are still on track