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

Removes configurability for resource indexer in solr indexing adapter #4222

Merged

Conversation

straleyb
Copy link
Contributor

Fixes #issuenumber ; refs #issuenumber

This PR pushes up some changes that had been discussed at Solar Vortex. There is not much of a reason to have a configurable indexer for the Indexing adapters. This is for the fact that indexing adapters will be the piece that is created and registered. So if someone wishes to use a new resource indexer in a specific adapter, they would either swap it or register a new indexer.

There are no tests that tested the configurability of the adapter as it was a way to create an ambiguous enough interface for testing purposes.

@samvera/hyrax-code-reviewers

@straleyb
Copy link
Contributor Author

Side question here. This test doesn't utilize these two lines. Was the goal of this to get it to use the persister to call save instead of call save on the adapter?

let(:persister) { Wings::Valkyrie::Persister.new(adapter: metadata_adapter) }
let(:metadata_adapter) { Wings::Valkyrie::MetadataAdapter.new }

@no-reply
Copy link
Contributor

Side question here. This test doesn't utilize these two lines. Was the goal of this to get it to use the persister to call save instead of call save on the adapter?

let(:persister) { Wings::Valkyrie::Persister.new(adapter: metadata_adapter) }
let(:metadata_adapter) { Wings::Valkyrie::MetadataAdapter.new }

looks like cruft to me. introduced here: 57450b5#diff-25d623799bda9453d7d1af9e8047932f

@straleyb
Copy link
Contributor Author

Not sure why ruby 2.5.5 failed and 2.4.7 is hanging. Ill keep an eye on this for any updates, but I think it should be good to go now.

@straleyb
Copy link
Contributor Author

@no-reply Should I run the tests again for the failed 2.5.5? The failures look unrelated.

@straleyb straleyb added randomly failing spec Known spec issue needing to be addressed and removed feedback needed labels Jan 27, 2020
@no-reply no-reply removed the randomly failing spec Known spec issue needing to be addressed label Jan 28, 2020
Copy link
Contributor

@no-reply no-reply left a comment

Choose a reason for hiding this comment

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

oops, just caught that this reader was left in place

@straleyb
Copy link
Contributor Author

@no-reply Got this back up to speed.

Transitions from using the instance variable to using the already included method that returns the resource indexer.
@straleyb
Copy link
Contributor Author

@no-reply I updated it to use that method to return the indexer. Definitely more hyrax-y and doesn't keep an unnecessary instance variable around. Good call on this one.

@no-reply no-reply merged commit fe7bd63 into master Jan 29, 2020
@no-reply no-reply deleted the feature/removeConfigurableIndexerForSolrIndexingAdapter branch January 29, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants