Skip to content
This repository

Review Flask-Sijax for approval. #317

Open
rduplain opened this Issue · 6 comments

3 participants

Ron DuPlain Slavi Pantaleev Alex Vykalyuk
Ron DuPlain
Collaborator

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).

Slavi Pantaleev

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.

Alex Vykalyuk

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

Slavi Pantaleev

Fixed. Thanks @alekzvik

Alex Vykalyuk

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

Slavi Pantaleev

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.