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

fix: before_first_request deprecation #440

Merged
merged 5 commits into from
Mar 21, 2024

Conversation

utnapischtim
Copy link
Contributor

No description provided.

Copy link
Member

@slint slint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some naming things.

return base_login(*args, **kwargs)


__all__ = ("blueprint", "login")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: I would also keep the blueprint for backwards compat.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know how, because the variable doesn't exist anymore in settings.py. that was the reason to move the def login function into settings.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and to be clear, as i understand it is also not possible to create this variable here, because i would have to call create_settings_blueprint here at a time where the current_app is not accessible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or should i do it that way, that i move the blueprint = Blueprint() outside of the create_settings_blueprint function?

@@ -75,7 +75,7 @@ def role_to_dict(role):
)


def create_blueprint(app):
def create_rest_blueprint(app):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def create_rest_blueprint(app):
def create_blueprint(app):

nit: since we're in rest.py anyways and this is not exported anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my thinking was, that i don't have to add lines like
from invenio_accounts.views.settings import create_blueprint as create_settings_blueprint in conftest.py, because i have to import both there. And in my opinion it looks clearer in the setup.cfg file for the entrypoints

check_security_settings(app)


def post_ext_init(app):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def post_ext_init(app):
def set_default_config(app):

nit: to be more descriptive, and maybe add a docstring as well

ACCOUNTS_BASE_TEMPLATE="invenio_accounts/base.html",
ACCOUNTS_SETTINGS_TEMPLATE="invenio_accounts/settings/base.html",
ACCOUNTS_COVER_TEMPLATE="invenio_accounts/base_cover.html",
WEBPACKEXT_MANIFEST_LOADER=MockManifestLoader,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment/question/shelve: wasn't it enough to just set this to None? If not maybe we should make it so, to make testing easier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tested it, and None is not enough because of following error message: AttributeError: 'NoneType' object has no attribute 'split'. i didn't tested if every variable has this problem, but to be consistent i would set all of them.

@utnapischtim utnapischtim marked this pull request as ready for review March 21, 2024 13:19
@kpsherva kpsherva merged commit c601010 into inveniosoftware:master Mar 21, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants