Allow multiple flask instances per Api instance #28
Conversation
| @@ -132,13 +140,18 @@ def _register_redoc_rule(self, blueprint): | |||
| redoc_url = ( | |||
| 'https://cdn.jsdelivr.net/npm/redoc@' | |||
| '{}/bundles/redoc.standalone.js'.format(redoc_version)) | |||
| self._redoc_url = redoc_url | |||
|
|
|||
There was a problem hiding this comment.
I'm not sure why you moved this function here. I think you could use current_app.name as in _openapi_json. Same thing goes for openapi_swagger_ui below.
There was a problem hiding this comment.
Thanks for reviewing.
I also need to pass redoc_url, swagger_ui_url and swagger_ui_supported_submit_methods.
I find it easier to use a closure than to pass them via the app object.
Or maybe I misunderstood your suggestion.
Are you suggesting I leave them as private attributes to the spec object (which always looked patchy to me, BTW) and I pass the spec object via the app object as app.extensions['flask-rest-api']['spec'] ?
There was a problem hiding this comment.
Sure. I haven't noticed those, sorry. I don't have a strong argument towards one approach or the other (I'm still familiarizing with your code).
| def _register_openapi_json_rule(self, app, blueprint): | ||
| """Serve json spec file""" | ||
| json_path = app.config.get('OPENAPI_JSON_PATH', 'openapi.json') | ||
|
|
There was a problem hiding this comment.
Similar to my other comment, I think you could use current_app instead of a closure with app. Except that here you may need to use: current_app._get_current_object() to access self.specs
There was a problem hiding this comment.
Yes, perhaps I could store the spec object in app.extensions['flask-rest-api']['spec'] and access it from there.
I don't have a strong feeling about it. Any downside with the closure approach?
There was a problem hiding this comment.
I don't have a strong argument against the closure approach. I just find a bit confusing having references to both current_app and app in the same function. I would pick one way to reference the application instance. Or maybe I'm missing something (?)
There was a problem hiding this comment.
Yeah, I agree it looks strange to use both. As I mentioned in the comment, I was using current_app here on the wrong assumption that current_app.response_class might be resolved at function definition time and would be wrong if the response class is changed after Api.init_app. I wan't sure about that but was too sloppy to check.
Here's the check:
def outside(obj):
def inside():
return obj.attr
return inside
class Obj:
def __init__(self, val):
self.attr = val
o = Obj(12)
f = outside(o)
assert f() == 12
o.attr = 42
assert f() == 42Great. So I can change that to
return app.response_class(
json.dumps(self.specs[app].to_dict(), indent=2),
mimetype='application/json')
lafrech
force-pushed
the
dev_allow_multiple_apps
branch
2 times, most recently
from
70ec56a
to
5b34024
Compare
lafrech
force-pushed
the
dev_allow_multiple_apps
branch
2 times, most recently
from
113f5e6
to
8a7ff4d
Compare
lafrech
force-pushed
the
dev_allow_multiple_apps
branch
from
8a7ff4d
to
b7d0985
Compare
lafrech
force-pushed
the
dev_allow_multiple_apps
branch
from
b7d0985
to
1c5b72a
Compare
lafrech
changed the title
|
This POC looks functional, but I'm still worried it might be opening the door to corner case issues. It changes the philosophy of the library. I'd like to see a convincing use case before pushing this further. Holding this for now. |
|
Awesome! Thanks for considering and coding this. Regarding use cases (I'm thinking at loud here...), I think it would be nice to have this in case you want to implement some kind of application dispatching combining various Flask application instances (http://flask.pocoo.org/docs/1.0/patterns/appdispatch/#application-dispatching). I will try to code some examples to see how that would actually work. cheers! |
|
Closing this for now. See #38 (comment). |
Closes #27.
TODO: