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

Set config through a config block instead of direct assignment #1089

Open
kevinnio opened this issue Jan 4, 2024 · 3 comments
Open

Set config through a config block instead of direct assignment #1089

kevinnio opened this issue Jan 4, 2024 · 3 comments

Comments

@kevinnio
Copy link
Contributor

kevinnio commented Jan 4, 2024

Hi there. Some colleagues and I were working with wicked_pdf and wkhtmltopdf-heroku when we noticed an issue. The latter gem adds a new value to WickedPdf.config[:exe_path] but the app was not getting it. Turns out our initializer overwrites the whole hash by doing WickedPdf.config = { ... }. I noticed the README says this is the recommended way to set config.

Of course the fix is simple enough but I wonder if something similar has happened to other devs. IMHO, there's a better way to set config by using a config block pattern:

# inside config/initializers/wicked_pdf.rb
WickedPdf.configure do |config|
  config[:page_size] = "Letter"
end

# inside any other gem:
WickedPdf.configure do |config|
  config[:exe_path] = CUSTOM_EXE_PATH
end

This way config is never completely overwritten, only specific keys, and values from other gems can be preserved and combined. We can also have default values if we need them.

What do you all think about this? I could implement this myself if you all feel this would be useful. We can still support WickedPdf.config = and even add a deprecation warning for it.

@unixmonkey
Copy link
Collaborator

This sounds like a good idea. I remember looking into it years ago, but it was never a priority. I would like to maintain backwards compatibility though.

@kevinnio
Copy link
Contributor Author

kevinnio commented Jan 5, 2024

First draft of my MR has been submitted.

@ghiculescu
Copy link

This can be closed since #1090 was merged.

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

No branches or pull requests

3 participants