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

Remove ConnectionAdapters::Resolver in favor of DatabaseConfigurations #37695

Merged

Conversation

@seejohnrun
Copy link
Member

seejohnrun commented Nov 12, 2019

We have these two objects, ConnectionAdapters::Resolver and
DatabaseConfiguratons that implement a lot of the same logic. One of
them is used for configurations defined in config/database.yml and the
other is used when passing raw configurations String or Hash objects
into methods like establish_connection.

Over time these two have diverged a bit. In the interest of less code
complexity, and more consistency for users this commit brings them back
together.

  • Remove Resolver altogether and replace its primary method with
    DatabaseConfigurations#resolve.

  • Move resolve_pool_config over to the ConnectionPool alongside the code
    that uses it.

cc / @eileencodes @tenderlove @rafaelfranca @etiennebarrie @casperisfine @Edouard-chin @matthewd

@rails-bot rails-bot bot added the activerecord label Nov 12, 2019
@seejohnrun seejohnrun force-pushed the seejohnrun:resolver-database-config-the-same branch 2 times, most recently from f23d823 to 963d6f1 Nov 12, 2019
We have these two objects, `ConnectionAdapters::Resolver` and
`DatabaseConfiguratons` that implement a lot of the same logic. One of
them is used for configurations defined in `config/database.yml` and the
other is used when passing raw configurations `String` or `Hash` objects
into methods like `establish_connection`.

Over time these two have diverged a bit. In the interest of less code
complexity, and more consistency for users this commit brings them back
together.

* Remove `Resolver` altogether and replace its primary method with
  `DatabaseConfigurations#resolve`.

* Move `resolve_pool_config` over to the `ConnectionPool` alongside the code
  that uses it.
@seejohnrun seejohnrun force-pushed the seejohnrun:resolver-database-config-the-same branch from 963d6f1 to 8d5a4ff Nov 13, 2019
Copy link

casperisfine left a comment

Definitely an improvement IMHO, 👍

@eileencodes eileencodes merged commit 3ab43e7 into rails:master Nov 13, 2019
1 of 2 checks passed
1 of 2 checks passed
build
Details
buildkite/rails Build #64905 failed (1 hour, 14 minutes, 1 second)
Details
@eileencodes eileencodes deleted the seejohnrun:resolver-database-config-the-same branch Nov 13, 2019
@eileencodes

This comment has been minimized.

Copy link
Member

eileencodes commented Nov 13, 2019

I'm not sure what's up with the yarn tests but it's not caused by these changes. I ran them locally and didn't see an issue.

seejohnrun added a commit to seejohnrun/rails that referenced this pull request Nov 13, 2019
This class has been removed in rails#37695 but I just noticed this lingering
around while doing some other work!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.