-
Notifications
You must be signed in to change notification settings - Fork 25
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
global: add support for index templates #148
Conversation
def register_index_templates(): | ||
"""Register search index templates for events.""" | ||
if not current_app.config["STATS_REGISTER_INDEX_TEMPLATES"]: | ||
return [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these functions look a bit too similar and I am worried they will be easily confusing, but I don't have a good idea on how to avoid this. Maybe a bit more elaborate comments on both to explain the backwards compatibility of register_templates
vs register_index_templates
?
also I guess, we cannot raise errors instead of empty list, because we lose the backwards compatibility ?
@@ -70,6 +70,8 @@ invenio_base.api_blueprints = | |||
invenio_stats = invenio_stats.views:blueprint | |||
invenio_search.templates = | |||
invenio_stats = invenio_stats.templates:register_templates | |||
invenio_search.index_templates = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any way of installing one or the other, not both? maybe we need to deprecate invenio_search.templates
?
9432027
to
ea4f6ba
Compare
After discussing with @slint IRL, we decided to drop the deprecation warning and add it only when we have a documented upgrade path. |
def register_templates(): | ||
"""Register search templates for events.""" | ||
if current_app.config["STATS_REGISTER_INDEX_TEMPLATES"]: | ||
return [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it makes sense to show a deprecation warning without having a "way out". Usually these warnings link to some documentation that explains why the change is happening and how to migrate to the new solution (see example below).
I think to move things forward it would be better to cut a minor release, and include this deprecation warning in a new PR that would also include this extra information in the text (and release as a minor/patch).
GH Actions deprecation warning
SQLAlchemy
SADeprecationWarning: When transforming <class '__main__.User'> to a
dataclass, attribute(s) "create_user", "update_user" originates from
superclass <class
'__main__.Mixin'>, which is not a dataclass. This usage is deprecated and
will raise an error in SQLAlchemy 2.1. When declaring SQLAlchemy
Declarative Dataclasses, ensure that all mixin classes and other
superclasses which include attributes are also a subclass of
MappedAsDataclass.
a49d5f9
to
18a5340
Compare
f0321c6
to
6f850d0
Compare
6f850d0
to
0d7db4e
Compare
❤️ Thank you for your contribution!
Description
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: