-
Notifications
You must be signed in to change notification settings - Fork 88
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
Dj/271 maintain one database connection #274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @drujensen
@@ -16,7 +16,7 @@ describe Granite::Adapters do | |||
|
|||
it "should disallow multiple adapters with the same name" do | |||
expect_raises(Exception, "Adapter with name 'mysql' has already been registered.") do | |||
Granite::Adapters << Granite::Adapter::Pg.new({name: "mysql", url: "PG_DATABASE_URL"}) | |||
Granite::Adapters << Granite::Adapter::Pg.new({name: "mysql", url: ENV["PG_DATABASE_URL"]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably doesn't matter, the test is expecting to catch an exception before the url is ever acted on
end | ||
|
||
def open(&block) | ||
yield DB.open(@url) | ||
yield database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
@robacarp FYI I've submitted PR to TFB to update Crystal and frameworks to the latest version TechEmpower/FrameworkBenchmarks#4020 The tests are passing now, but I think I should maybe wait for this fix in Granite ORM for better benchmarks results for Amber? Let me know if there is a bugfix release for Granite ORM planned soon, so I can update my PR before it gets merged in. |
@vlazar I will release this today. |
This fixes the connection issue #271