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

Review Flask-Sijax for approval. #317

Closed
rduplain opened this issue Sep 6, 2011 · 7 comments
Closed

Review Flask-Sijax for approval. #317

rduplain opened this issue Sep 6, 2011 · 7 comments
Assignees

Comments

@rduplain
Copy link
Contributor

rduplain commented Sep 6, 2011

Slavi Pantaleev
http://packages.python.org/Flask-Sijax/

@ghost ghost assigned rduplain Sep 6, 2011
rduplain added a commit that referenced this issue Nov 24, 2011
@rduplain
Copy link
Contributor Author

Listed. Recommendation: move the init_app and route functions into methods of the SijaxHelper class, and consider shortening the name for SijaxHelper to just Sijax. These changes would lead to an API that is more consistent with Flask extension patterns.

I'm testing with Flask 0.8, and test_sijax_helper_object_is_only_bound_to_g_in_a_request_context errors out with RuntimeError: working outside of request context.

Python 2.5 breaks, starting with lack of JSON support (json was added in Python 2.6, based on simplejson package).

@spantaleev
Copy link

Thanks for the suggestions.

The test was broken, because of a change in Flask 0.7 - this is now fixed.

I renamed the class from SijaxHelper to Sijax and changed things a bit, so it implements the init_app pattern seen in other extensions (the init_sijax module function is now gone).
The whole thing was moved to the flask_sijax package, getting rid of the flaskext.sijax namespaced package.

I kept the route helper decorator at the module level (currently flask_sijax.route) as it didn't seem right to have it as a static method in the Sijax class or even more so as an instance method.

What do you suggest should be done about Python 2.5 and JSON? The Sijax library throws a RuntimeError the first time json is used, if it couldn't find a json implementation ( https://github.com/spantaleev/sijax-python/blob/eb2c8fbb92386b361093863692a4ec4a4ca7e2cb/sijax/helper.py#L17 ). If simplejson has to be listed as a requirement in setup.py maybe that needs to happen in Sijax anyway, and not in this Flask extension.

@alekzvik
Copy link
Contributor

alekzvik commented Apr 4, 2012

You can add simplejson as requirement to sijax only when needed - in Python < 2.6.
Useful example:
coleifer/flask-peewee@eaf9059

@spantaleev
Copy link

Fixed. Thanks @alekzvik

@alekzvik
Copy link
Contributor

alekzvik commented Apr 5, 2012

You're welcome.
btw, why did you put all the code in init.py?

@spantaleev
Copy link

Because the extension seems small enough (~200 lines) and splitting it into modules didn't seem justified.

If you're asking why I didn't package a single module file (flask_sijax.py), instead of a "one file package", I guess that would be possible now. It had to be a package at the time Flask encouraged namespaced packages for extensions.

@untitaker
Copy link
Contributor

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
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

4 participants