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

Require hyrax/search_state #3472

Closed
wants to merge 1 commit into from
Closed

Require hyrax/search_state #3472

wants to merge 1 commit into from

Conversation

cjcolvar
Copy link
Member

When attempting to set the admin_set_create_service for the AdminSetsController in the hyrax initializer, you get an uninitialized constant error for Hyrax::SearchState. It is required in an initializer within the Hyrax engine but for some reason that hasn't been loaded by this point in the hyrax initializer. This commit makes the dependency explicit by requiring the load of search state here when Hyrax::Controller is used.

@samvera/hyrax-code-reviewers

When attempting to set the admin_set_create_service for the AdminSetsController in the hyrax initializer, you get an uninitialized constant error for Hyrax::SearchState.  It is required in an initializer within the Hyrax engine but for some reason hasn't been loaded by that point in the hyrax initializer.  This commit makes the dependency explicit by requiring the load of search state here.
@cjcolvar
Copy link
Member Author

FWIW The two failed travis builds had all of their rspec tests pass.

@no-reply
Copy link
Contributor

This seems okay to me, but I'm not sure I'm understanding why the relevant requires need to be in an tantalizer at all. I'm going to try simply moving them out to the main body of the Hyrax::Engine

no-reply pushed a commit that referenced this pull request Jan 25, 2019
This loads dependencies from `lib/hyrax` and dry-rb explictly from
Hyrax::Engine, instead of waiting until the initializer.

This is an alternate implementation to the one in
#3472
@no-reply
Copy link
Contributor

@cjcolvar
Copy link
Member Author

That approach looks good to me. The only reason I can think of to load those later in an initializer is to help speed up the load time or because of some load order issue but those both seem unlikely and if they work outside of the initializer then I'm 👍.

@cjcolvar
Copy link
Member Author

@no-reply Do you have time to rebase that branch and open a PR or would you like me to do that?

@stale
Copy link

stale bot commented Mar 15, 2019

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

@stale stale bot added the stale label Mar 15, 2019
cjcolvar pushed a commit that referenced this pull request Mar 19, 2019
This loads dependencies from `lib/hyrax` and dry-rb explictly from
Hyrax::Engine, instead of waiting until the initializer.

This is an alternate implementation to the one in
#3472
hackartisan pushed a commit that referenced this pull request Mar 20, 2019
This loads dependencies from `lib/hyrax` and dry-rb explictly from
Hyrax::Engine, instead of waiting until the initializer.

This is an alternate implementation to the one in
#3472
@cjcolvar
Copy link
Member Author

Closed by #3686.

@cjcolvar cjcolvar closed this Mar 22, 2019
@cjcolvar cjcolvar deleted the cjcolvar-patch-1 branch March 22, 2019 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants