Skip to content
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

Make AccessToken, RefreshToken and Grant models swappable. #467

Closed
wants to merge 27 commits into from

Conversation

menecio
Copy link

@menecio menecio commented Mar 27, 2017

Make it possible to use custom models for access tokens, refresh tokens and grants.

@menecio
Copy link
Author

menecio commented Mar 27, 2017

I started a clean branch after the original got abandoned. #347

@jleclanche
Copy link
Member

At first glance it looks a ton better than before. I think this will be mergeable.

I'll try to find some time to review this. but I'm a bit short on it right now. @clintonb wanna help?

@menecio
Copy link
Author

menecio commented Mar 28, 2017

@jleclanche I added more test cases, updated docs and cleaned a bit. Please let me know any questions or suggestions you might have.

hi @clintonb

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 92.205% when pulling 8ca8dea on menecio:master into 4e89234 on evonove:master.

@jleclanche
Copy link
Member

This is great, but it'll take me a while to get to it and review. It definitely looks landable though, we can get this in for next version.

@menecio
Copy link
Author

menecio commented Mar 29, 2017

This PR #252 is also related to this. I didn't know this was a 3rd attempt to get this feature.

@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage increased (+0.3%) to 93.554% when pulling b9d1443 on menecio:master into cb5b231 on evonove:master.

@menecio
Copy link
Author

menecio commented Apr 13, 2017

@jleclanche I just updated this to with the latest changes merge into master

@jleclanche
Copy link
Member

@menecio can you rebase it instead of merge? This still has conflicts as-is.

@coveralls
Copy link

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.3%) to 93.554% when pulling c35ae2f on menecio:master into cb5b231 on evonove:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 93.554% when pulling c35ae2f on menecio:master into cb5b231 on evonove:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 93.554% when pulling c35ae2f on menecio:master into cb5b231 on evonove:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 93.554% when pulling c35ae2f on menecio:master into cb5b231 on evonove:master.

@menecio
Copy link
Author

menecio commented Apr 20, 2017

@jleclanche Done. Let me know if there is something else. Thank you very much for your time.

@menecio
Copy link
Author

menecio commented Apr 24, 2017

hey @clintonb I'll ping you to see if you have some time to check this one out. @jleclanche seems to be quite busy.

Thanks

@jleclanche
Copy link
Member

Apologies, fell through the crack. Reviewing now.

@jleclanche
Copy link
Member

@menecio Landed in master with some minor cleanups.

And I just want to thank you for this PR. Not only is it a feature that's been long requested in DOT, but you implemented it cleanly, pushed out well-separated commits, made it really easy to review and just generally kept it high quality. In 10 years of using Github, I can count on one hand the amount of times I've seen this happen for drive-by PRs :)

@bunchesofdonald
Copy link

Thank you @coveralls! @jleclanche or @clintonb are there any plans to release this to pypi in the near future?

@clintonb
Copy link
Contributor

clintonb commented May 8, 2017 via email

@jleclanche
Copy link
Member

Yeah I can't do releases either.

@bunchesofdonald
Copy link

Sorry to have bothered you guys! It looks like maybe it's @synasius that can do a release?

@menecio
Copy link
Author

menecio commented May 11, 2017

It would be nice to have some feedback from @synasius about a possible release of this.

@danadn
Copy link

danadn commented Jun 28, 2017

Any news on this? @synasius would be great to use JWT as access_token (#252 leads here). Thanks.

@jleclanche
Copy link
Member

The release happened a while ago.

@danadn
Copy link

danadn commented Jun 28, 2017

Ok, I was using a previous release.

Shame on me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants