Review Flask-Peewee for listing, then approval. #328

Closed
rduplain opened this Issue Sep 23, 2011 · 10 comments

4 participants

@rduplain rduplain was assigned Sep 23, 2011
@rduplain

I'll first review the extension for listing based on Flask extension patterns, then open the floor for those in the community to review the extension for approval.

@coleifer

Thanks for opening the issue for me, wasn't sure where to go after bugging you guys in IRC!

@rduplain

You have the correct process:

  • announce extension to other developers on mailing list and/or IRC
  • request core team to review for listing and approval

I post the issues here so that we can be transparent about extensions which are under review, and to keep a clear backlog of what needs to be done.

@rduplain rduplain added a commit that referenced this issue Nov 24, 2011
@rduplain rduplain List Flask-Peewee, #328. fe7b725
@rduplain

Code organization and project layout overall looks excellent. I'm listing the extension now, which based on your commit log was ready for listing some time ago (for listing, I'm only looking for stable packaging and clear public APIs). For an approved listing, Flask-Peewee does not yet meet guidelines 1, 3, and 10.

For 1, it has many modules inside flaskext, but should instead have everything inside the flask_peewee (preferred) or flaskext.peewee (legacy) namespace. For the flask_peewee case, it would not need to be a namespace package. The shortest fix is to rename flaskext as it stands now to flask_peewee.

For 3, Flask-Peewee needs to support application factories. This should be just a matter of adding Database.init_app and having the Database __init__ work without an app object. Of course, init_app would need to be called before Database could do it's work. See the extenstiondev doc, further up from the guidelines for details on init_app, and refer to Flask-SQLAlchemy if needed.

For 10, Python 2.5 breaks, starting with the with statement. (need to import with_statement)

http://flask.pocoo.org/docs/extensiondev/#approved-extensions

PS - I was reviewing against a checkout of code from last night -- I see a lot of Flask-Peewee activity today.

@coleifer

Thanks for your notes, they were very helpful and I will be working to address the issues you've mentioned.

  1. Have renamed and pushed new version that does a normal python package called flask_peewee.
  2. I find it a little odd that this is a requirement, it seems like if something in my extension depends on an application object then it shouldn't be possible to instantiate without it. To make it not depend on an application object is to subvert the intent/purpose of the object. The implementation seems gross, too. To avoid circular imports in my own projects, I structure them like I do in the example app that ships with flask_peewee.
  3. Added from future statements in relevant modules
@alekzvik

Python 2.5 still breaks
You use json module in admin.py, but it is available only from Python 2.6.
I think you should add simplejson to the dependencies.

@coleifer

I believe this should fix it:
coleifer/flask-peewee@eaf9059

@alekzvik

Yes, problem solved.

@coleifer

Hey, just wanted to let you know the docs url on http://flask.pocoo.org/extensions/ is incorrect.

Should be:

http://flask-peewee.readthedocs.org/

@untitaker
The Pallets Projects member

please refile against https://github.com/pocoo/metaflask

@untitaker untitaker closed this Oct 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment