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

Adapter specific configuration #225

Open
wingrunr21 opened this issue Sep 2, 2015 · 6 comments
Open

Adapter specific configuration #225

wingrunr21 opened this issue Sep 2, 2015 · 6 comments
Assignees
Milestone

Comments

@wingrunr21
Copy link
Collaborator

Wanted to open an issue on this before putting together a PR.

I've got a use case where a user would like a configuration option for the Mandrill adapter (see wingrunr21/griddler-mandrill#23). Right now I am considering three approaches:

  • Add some kind of adapter_config attribute to Gridder::Configuration which is passed into new in Griddler::EmailsController. The adapter_config would be an arbitrary hash that the adapter would then consume as needed (in my case configuring SPF checks).
  • Allow the email_service config option to accept an optional hash of additional configuration. The rest would function like the adapter_config
  • Add a new configuration object in the adapter itself. I am less partial to this as it feels a little weird having the adapter configure itself in addition to the parent project.

Any thoughts on this?

@gabebw
Copy link
Contributor

gabebw commented Sep 2, 2015

If I understand your first bullet point correctly, then this line changes from

processor_class.new(email).public_send(processor_method)

to

if extra_configuration?
  processor_class.new(email, extra_configuration).public_send(processor_method)
else
  processor_class.new(email).public_send(processor_method)
end

(I added the if so that adapters that only take 1 arg don't break.) I like that version the best.

@wingrunr21
Copy link
Collaborator Author

Ya, that's what I meant.

We can also check the arity of new to protect adapters or increment the griddler version (since this is an API change) and just have adapters depend on the older version until they update.

@gabebw
Copy link
Contributor

gabebw commented Sep 2, 2015

increment the griddler version (since this is an API change)

Yes, good point. I like that idea better, since as you point out, this is a breaking change.

@micahbf
Copy link

micahbf commented Oct 25, 2017

Would love to see this merged! I ran into the exact issue @wingrunr21 describes - we need to be able to configure SPF checking in griddler-mandrill. In the mean time we will probably need to maintain our own fork of the adapter gem.

@wingrunr21
Copy link
Collaborator Author

@micahbf agreed, I need to get back on getting this change pushed out. I'm a bit swamped right now but I will try and find time.

@micahbf
Copy link

micahbf commented Oct 25, 2017

@wingrunr21 Let me know if I can help.

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

No branches or pull requests

3 participants