Skip to content

Check to see if being run in Rails application and run in before_initialize - #555

Merged
andrehjr merged 2 commits into
splitrb:masterfrom
husteadrobert:run_configure_after_initialize_on_rails
Mar 27, 2019
Merged

Check to see if being run in Rails application and run in before_initialize#555
andrehjr merged 2 commits into
splitrb:masterfrom
husteadrobert:run_configure_after_initialize_on_rails

Conversation

@husteadrobert

Copy link
Copy Markdown

Addresses isssue #528

Checks to see if Rails::Railtie is defined, and if so sets the Railtie to run before_initialize. I've run this locally with no issues and specs are passing.

Comment thread lib/split.rb Outdated

Split.configure {}
# Check to see if being run in a Rails application. If so, wait until before_initialize to run configuration so Gems that create ENV variables have the chance to initialize first.
if defined?(Rails::Railtie)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to check for Rails instead?

if defined?(::Rails)

See other gems for example: https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/rails.rb#L51 and https://github.com/brandonhilkert/sucker_punch/blob/master/lib/sucker_punch.rb#L57

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing! I thought being explicit (since we will be inheriting from Railties) was better, but seeing the convention of checking for ::Rails seems like a good idea. Updated!

@andrehjr

Copy link
Copy Markdown
Member

Thanks for the PR @husteadrobert! I left a small comment, after that I'll merge it :)

@husteadrobert

Copy link
Copy Markdown
Author

I don't seem to be able to re-request a review, so I'm leaving a comment here. If you need any other changes, please let me know! @andrehjr

@andrehjr

Copy link
Copy Markdown
Member

Thanks @husteadrobert with this PR, it should be possible to use Figaro :)

@andrehjr
andrehjr merged commit 7eb8f26 into splitrb:master Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants