8000 Support specifying 'url' in configuration files by lfittl · Pull Request #106 · TalentBox/sequel-rails · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support specifying 'url' in configuration files #106

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

Merged
merged 1 commit into from
Apr 28, 2016

Conversation

lfittl
Copy link
@lfittl lfittl commented Apr 26, 2016

Right now this works fine for all Sequel connections, but breaks for all rake tasks since the URL isn't parsed. This ends up being a deal breaker when using sequel-rails with a config/database.yml that looks like this:

...
development:
  url:  <%= ENV['DATABASE_URL'] %>
...

This adds support to the Storage helpers, which fixes the rake tasks.

Previously this worked fine for all Sequel connections, but broke
for all rake tasks since the URL wasn't parsed. This adds support
to the Storage helpers, which fixes the rake tasks.
@@ -12,10 +12,9 @@
},
'test' => {
'adapter' => 'postgres',
'url' => 'postgres://127.0.0.1/sequel_rails_test_storage_test',
Copy link
Author

Choose a reason for hiding this comment

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

It was a bit hard for me to find a good place to add the Storage::Abstract tests - let me know where you think that'd be appropriate.

@lfittl
Copy link
Author
lfittl commented Apr 28, 2016

@JonathanTron Sorry to bug you directly, but: any feedback on this one?

Would be great to see full url/DATABASE_URL support when using Sequel+Rails :)

@JonathanTron
Copy link
Member

Hi @lfittl, thanks for the PR and sorry for the delayed response.

It is indeed not handled properly in the current code and you PR fix this.
I'm not fan of using try, but this is only nitpicking that I can update after merging.

Thanks a lot for your work.

@JonathanTron JonathanTron merged commit 3a03673 into TalentBox:master Apr 28, 2016
@lfittl
Copy link
Author
lfittl commented Apr 28, 2016

@JonathanTron Yay, thanks for merging - happy that I don't have to keep a fork around :-)

@JonathanTron
Copy link
Member

@lfittl You're welcome.

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.

2 participants
0