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

Add configuration class for custom adapters #135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mateuscruz
Copy link

The list of supported adapters is hardcoded, which makes it difficult to use this gem on custom adapters.

We use postgresql_proxy from active_record_proxy_adapters, for instance. Currently, we need to monkey patch the gem and hot swap the constant HairTrigger::POSTGRESQL_ADAPTERS when the application boots to make it work with the proxy adapter.

Although it works, it's not ideal.

I've added a new configuration class whose values will default to the current ones (%i[mysql2 trilogy] for mysql_adapters, %i[postgresql postgis] for postgresql_adapters , and %i[sqlite litedb] for sqlite_adapters), but allow the user to add custom adapters for each.

I don't expect any breaking changes here as the values fall back to the existing constants, but please double check. Thanks!

The list of supported adapters is hardcoded, which makes it difficult to use this gem on custom adapters.

We use `postgresql_proxy` from `active_record_proxy_adapters`, for instance.
Currently, we need to monkey patch the gem and hot swap the constant `HairTrigger::POSTGRESQL_ADAPTERS`  when the application boots to make it work with the proxy adapter.

Although it works, it's not ideal.

I've added a new configuration class whose values will default to the current ones (`%i[mysql2 trilogy]` for `mysql_adapters`, `%i[postgresql postgis]` for `postgresql_adapters` , and `%i[sqlite litedb]` for `sqlite_adapters`), but allow the user to add custom adapters for each.

I don't expect any breaking changes here as the values fall back to the existing constants.
@mateuscruz
Copy link
Author

@jenseng could you please review this when you have a chance?

@mateuscruz
Copy link
Author

@jenseng bump

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

Successfully merging this pull request may close these issues.

1 participant