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

Encapsulate the blacklight config #2083

Merged
merged 2 commits into from Jan 19, 2022
Merged

Encapsulate the blacklight config #2083

merged 2 commits into from Jan 19, 2022

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Mar 7, 2019

All values set on Blacklight::Engine.config are actually stored
in a class variable on Railtie::Configuration. So these pollute
that namespace. All other railties are encapsulating their configs
onto a single entry in that scope. This change makes Blacklight do
likewise.

This probably needs to be a major revision

@mejackreed mejackreed added this to the 8.x milestone Oct 8, 2019
@bess
Copy link
Member

bess commented Oct 11, 2019

+1 but it seems like we should talk about when to merge this, if we think it's a major version increment. Would it make sense to mark this as a Blacklight 8 feature?
UPDATE: And I see that @mejackreed already tagged this for 8.x. Thanks!

@cbeer cbeer moved this from Backlog to Needs Major Version in Blacklight Summit Triage Board Nov 9, 2021
@barmintor
Copy link
Contributor

Now that the 8.0.0.alpha tag is in main, can we revive this PR?

All values set on Blacklight::Engine.config are actually stored
in a class variable on Railtie::Configuration. So these pollute
that namespace.  All other railties are encapsulating their configs
onto a single entry in that scope.  This change makes Blacklight do
likewise.

This probably needs to be a major revision
@barmintor
Copy link
Contributor

Rebasing for CI before merge.

@barmintor
Copy link
Contributor

The OpenStruct linter rule was added in the interim of this PR being opened. There's three approaches available to us:

  1. Use OpenStructWithHashAccess here, and remove it (or not) along with the rest of the usages of that class
  2. Add to rubocop_todo
  3. Disable the Style/OpenStructUse linter rule.

I think there's an argument for disabling the rule rather than adding yet another todo, but I'm also sympathetic to using OpenStructWithHashAccess and dealing with that (or not!) as a single task in the future.

@tpendragon
Copy link
Member

rubocop/rubocop#10206 is where they added this, and it makes a good point that Ruby 3 basically says "quit it."

I'm for option 1, and actually removing that class one day (although I suspect it's gonna be hard :( )

@barmintor
Copy link
Contributor

Sold @tpendragon. I'll merge after CI.

@barmintor barmintor merged commit a2a1a68 into main Jan 19, 2022
@barmintor barmintor deleted the encapsulate_config branch January 19, 2022 01:05
@cbeer
Copy link
Member

cbeer commented Jan 19, 2022

@jcoyne or @barmintor, can you create a PR against the release-7.x with appropriate deprecations and a migration path for this change?

@jcoyne
Copy link
Member Author

jcoyne commented Jan 19, 2022

@cbeer I wouldn't know how to do that as Blacklight::Engine.config is pointing at the Railtie::Configuration. Since that's not a class we control, we can't add deprecations to it.

@barmintor
Copy link
Contributor

It's true that it might be awkward, but I created #2607 to try to work through it @cbeer

@barmintor
Copy link
Contributor

Ok, I have a proposed deprecation up in #2610

Spoiler: It was kind of awkward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants