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 #171

Merged
merged 5 commits into from
Mar 22, 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.

Overall looks clean and as you mentioned pretty simple (even for more complex cases like this one). I guess there's always a tiny bit of modifications needed in keeping context/variables on the extension object, but it looks clean.

@@ -259,8 +260,7 @@ def get_context(self, pid_value=None):
serialized_schema = self._schema_to_json(schema)
fields = self.item_field_list
return {
"request_headers"
"name": name,
"request_headers" "name": name,
Copy link
Member

Choose a reason for hiding this comment

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

That looks like a bug actually 😅
@kpsherva (or anyone familiar), can you verify?

Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of introducing a new filename convention for these type of functions. Could we maybe instead leave these functions ext.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.

if i move all what it is now in finalize_app.py should i keep the function structure the same. so one function def finalize_app as the entrypoint within the ext.py file?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think splitting into multiple entrypoints doesn't achieve anything, so one big function calling smaller functions doing their thing, looks like the way to go. Then the entrypoint is just pointing to invenio_administration.ext:finalize_app.

* the deprecation warning has been fixed by using the new entry_point
  invenio_base.finalize_app
* the problem was that the mock fixtures were called after the app creation.
  this does not work because in the finalize_app they have to be ready already.
  therefore they are now called as a entry point before of the finalize_app
* invenio-accounts, invenio-theme, invenio-base, invenio-app
@utnapischtim utnapischtim marked this pull request as ready for review March 21, 2024 22:16
@kpsherva kpsherva merged commit 7e3d670 into inveniosoftware:main Mar 22, 2024
16 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