fix: before_first_request deprecation #303
Conversation
| @@ -39,28 +39,6 @@ | |||
| ) | |||
|
|
|||
|
|
|||
| @blueprint.record_once | |||
There was a problem hiding this comment.
One thing I just realized, in this case this function using @blueprint.record_once would only run for the UI app and not the API app (i.e. the /api mount). Some of these cases are intentional (like here for example, we're setting template config variables). For other cases it might be though something that was overlooked or a even mistake.
This might hint that we need a UI and API variant of the "finalize_app" entrypoint? Similar to how we have invenio_base.blueprints and invenio_base.api_blueprints.
There was a problem hiding this comment.
should i have first a look, if there are already use-cases for invenio_base.api_finalize_app?
There was a problem hiding this comment.
I think it's good to have it and be conscious about its usage, so yes I mean it's meant to happen at some point that we need something running on the API app only.
There was a problem hiding this comment.
the commit in invenio-app which introduce the invenio_base.finalize_app entrypoint is only applied to create_ui and create_app and not create_api. Should the invenio_base.api_finalize_app entrypoint then added to create_api?
the /api endpoint is mentioned in create_app. does this mean that invenio_base.api_finalize_app should be added to create_app instead of invenio_base.finalize_app? As i read that code, that would be false, so it should remain as it is with invenio_base.finalize_app.
invenio_oauthclient/finalize_app.py
Outdated
|
|
||
| def init_index_menu(app): | ||
| """Init index menu.""" | ||
| item = current_menu.submenu("settings.oauthclient") |
There was a problem hiding this comment.
Isn't the use of current_menu here wrong? Since we're not inside the application context at this point AFAIK.
Wouldn't the alternative be to either call app.extensions['menu'].submenu(...) or have the un-initialized Menu extension to import from invenio-theme?
There was a problem hiding this comment.
hm, but the code as it is is working, but it would be more understandable if i we change to app.extensions['menu'].submenu(...)
There was a problem hiding this comment.
both variants current_menu and app.extensions["menu"].submenu(...) are working as expected. i tested it by changing the text and checking the value on the page (not the best way, but for now i think enough). further i thought that current_menu has to work, because otherwise the application would not start.
invenio_oauthclient/finalize_app.py
Outdated
| ) | ||
|
|
||
|
|
||
| def init_index_breadcrumb(): | ||
| """Init index breadcrumb.""" | ||
| item = current_menu.submenu("breadcrumbs.settings.oauthclient") | ||
| item.register("invenio_oauthclient_settings.index", _("Linked accounts")) |
There was a problem hiding this comment.
IMO, menus and breadcrumbs go together, so I would combine these two functions.
There was a problem hiding this comment.
maybe i will write they in a dict and do it with for loop. it is sometimes a little bit difficult to read, if everything is written out.
There was a problem hiding this comment.
I'm fine with anything as long as it reads clean :)
It's more about the fact that so far usually wherever I see a register_menu, there's also register_breadcrumb, so it kind of makes sense to do these together.
utnapischtim
force-pushed
the
add-init-function
branch
from
ef6032d
to
df989b5
Compare
utnapischtim
force-pushed
the
add-init-function
branch
from
df989b5
to
b7d57c9
Compare
* sphinx deprecation warning about wrong intersphinx_mapping usage * initialize InvenioI18N was missing * add missing configuration variable
utnapischtim
force-pushed
the
add-init-function
branch
from
b7d57c9
to
000c21f
Compare
* move implicit use to explicit import * adapt usage of changed invenio-userprofiles programmatic api * remove unused import * since the actual way to build the test app is not used, the finalize-app functions have to be executed manually. in this particular case the call of the finalize_app of invenio-userprofiles was missing.
utnapischtim
force-pushed
the
add-init-function
branch
from
de87fc7
to
9e1b149
Compare
invenio-theme, flask-menu, invenio-accounts, invenio-userprofiles