Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

SQLAlchemy Storage #345

Closed
miedzinski opened this issue Nov 30, 2015 · 10 comments
Closed

SQLAlchemy Storage #345

miedzinski opened this issue Nov 30, 2015 · 10 comments

Comments

@miedzinski
Copy link
Contributor

#319

I've started working on SQLAlchemy Storage class, based on AppEngine and Django ones. There is already functional class, without unit tests. I am not sure whether more docs than docstrings are needed.

Here is the fork:
https://github.com/miedzinski/oauth2client/tree/sqlalchemy-storage

@theacodes
Copy link
Contributor

We have two efforts in place to make this easier:

  1. Create oauth2client.contrib package for non-core modules. #326 to add oauth2client.contrib as a place to keep these items.
  2. [FR] Useful storage for web applications. #319 to add a few storage classes for web applications.

I'm waiting on #344 to be reviewed and merged before creating the contrib package.

@miedzinski
Copy link
Contributor Author

Sure, I don't see moving existing class to another file a problem. When I'll be ready to submit PR (a.k.a. write tests), I'll check if contrib package exists - and then move class to this file.
Just wanted to tell people that I'm already working on this class, because it's needed in my application.

@theacodes
Copy link
Contributor

No worries. I think it's very useful, especially if you/we can get it to work in conjunction with flask_util.

@miedzinski
Copy link
Contributor Author

Well, my application uses Flask as well, but flask_util needs changes in order to support storage like this (or Django ORM). There should be a way to pass arguments to Storage constructor.
Anyway, I'm open to ideas regarding design of current oauth2client.sqlalchemy.storage class. Currently the user passes a session to it and has to flush/commit manually (assuming engine's auto-commit/flush is disabled).

edit:
After looking it up, I see UserOAuth2 actually takes Storage instance, so it should be okay.

@anthonymayer
Copy link

Any updates on this? I'd like to be able to use this and it'd be great if it were just part of the library. Thanks.

@theacodes
Copy link
Contributor

contrib is now available and I'm happy to review new modules to be added there.

@miedzinski
Copy link
Contributor Author

I have rebased my branch to master, but it is lacking some tests. Maybe we could merge it to some kind of dev branch? If not, I need few days to write them, but until them you can try using my fork.

@theacodes
Copy link
Contributor

We don't have a dev branch, so just send a PR from your fork when you're ready for review. We do have a strict 100% code coverage requirement.

@miedzinski
Copy link
Contributor Author

@anthonymayer, good news! I have submitted #527.

@miedzinski
Copy link
Contributor Author

@anthonymayer, good news again. PR was merged -- 267bbc5.

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

No branches or pull requests

3 participants