-
Notifications
You must be signed in to change notification settings - Fork 124
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
remove the indexer initializer generator #4294
Conversation
the code this generator injects isn't really necessary (if we want to register this indexer for the generic `Hyrax::Work`, we can do it in Hyrax directly). including it complicates the upgrade process for existing apps. instead, while we need to manually register indexers for specific works (maybe we can do this with configuration soon, instead?), inject them into the hyrax initializer when the work generator runs.
when we generate valkyrie works with hyrax basic metadata, so we should also generate the indexer with that schema loaded in.
@@ -16,6 +16,7 @@ | |||
require 'hyrax/engine' | |||
require 'hyrax/version' | |||
require 'hyrax/inflections' | |||
require 'hyrax/name' |
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 unclear why this is added.
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.
it turned out that actually running this generator and starting an app caused NameError
on this module. it seems like it should have just always been there.
@@ -3,7 +3,7 @@ | |||
# Generated via | |||
# `rails generate hyrax:work_resource <%= class_name %>` | |||
class <%= class_name %>Indexer < Hyrax::ValkyrieWorkIndexer | |||
Hyrax::ValkyrieIndexer.register self, as_indexer_for: <%= class_name %> | |||
include Hyrax::Indexer(:basic_metadata) |
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.
With this change, will those that ran previous generators need to change their registration logic? What is the process for people to do that? Do we have documentation in the upgrade notes?
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.
With this change, will those that ran previous generators need to change their registration logic?
yes.
What is the process for people to do that? Do we have documentation in the upgrade notes?
this code has never been released (even in an RC), so i think we might be in the clear here? i'm open to counter views on that point, though.
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 guess i'll also add: they only need to make this change if they want :basic_metadata
.
if they do want that, it's likely they've already made this change.
the code this generator injects isn't really necessary (if we want to register
this indexer for the generic
Hyrax::Work
, we can do it in Hyraxdirectly). including it complicates the upgrade process for existing
apps. instead, while we need to manually register indexers for specific
works (maybe we can do this with configuration soon, instead?), inject them into
the hyrax initializer when the work generator runs.
additionally, generate basic metadata into the indexer.
@samvera/hyrax-code-reviewers