Skip to content

Follow up #33637 and #33770#33825

Merged
eileencodes merged 3 commits intorails:masterfrom
bogdanvlviv:follow-up-33637-and-33770
Sep 9, 2018
Merged

Follow up #33637 and #33770#33825
eileencodes merged 3 commits intorails:masterfrom
bogdanvlviv:follow-up-33637-and-33770

Conversation

@bogdanvlviv
Copy link
Contributor

@bogdanvlviv bogdanvlviv commented Sep 8, 2018

  • Fix sqlite3 url config in "Configuring Rails Applications" guide
    See ConnectionUrlResolver#database_from_path in
    activerecord/lib/active_record/connection_adapters/connection_specification.rb

  • Fix explanation of ActiveRecord::Base.configurations.configs_for in the CHANGELOG
    Since Part 3: Multi-db Improvements, identifying replica configurations #33770 #configs_for changed method signature and it isn't
    supposed to work with a passed block.

  • Fix ActiveRecord::DatabaseConfigurations's docs

  • Allow ActiveRecord::DatabaseConfigurations#configs_for receive :env_name and spec_name as symbol

  • Remove extra reverse in ActiveRecord::DatabaseConfigurations#to_h

r? @eileencodes

See `ConnectionUrlResolver#database_from_path` in
 `activerecord/lib/active_record/connection_adapters/connection_specification.rb`
… the CHANGELOG

Since rails#33770 `#configs_for` changed method signature and it isn't
supposed to work with a passed block.
@bogdanvlviv bogdanvlviv force-pushed the follow-up-33637-and-33770 branch from 6fe2de1 to ecf6f30 Compare September 9, 2018 13:04
@bogdanvlviv bogdanvlviv force-pushed the follow-up-33637-and-33770 branch from ecf6f30 to 2bdca99 Compare September 9, 2018 13:12
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the docs. I left 2 comments about changes that I'd like to be reverted.

Also fyi the APIs for multi-db stuff is a moving target and is going to evolve a lot over the next couple of months as I add more features. It might be better to wait to do a full audit of the docs when I'm done instead of constantly having to clean them up 😄

Copy link
Member

Choose a reason for hiding this comment

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

Can you put both of these back? They are not there accidentally. I'll add a test showing why they both need to be there but the gist is if the first reverse isn't there a multi-db tier will return the last configs instead of the first configs (ie animals instead of primary) and the second one was just for parity with Rails 5.2 - that it will return the config hash in the order of the database.yml. This is for backwards compatibility and will be removed in 6.1.

Copy link
Member

Choose a reason for hiding this comment

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

Can you undo the changes that allow non-strings? I don't want the configs_for api to accept symbols. Maybe I'll change my mind later on but this API is a WIP so I'd rather just make it relatively strict for now.

@bogdanvlviv bogdanvlviv force-pushed the follow-up-33637-and-33770 branch from 2bdca99 to 0a6aa42 Compare September 9, 2018 14:20
@bogdanvlviv
Copy link
Contributor Author

bogdanvlviv commented Sep 9, 2018

@eileencodes Thank you for the review. I just removed
Allow ActiveRecord::DatabaseConfigurations#configs_for receive :env_name and spec_name as symbol, and Remove extra reverse in ActiveRecord::DatabaseConfigurations#to_h commits.

@eileencodes eileencodes merged commit 101096e into rails:master Sep 9, 2018
@bogdanvlviv bogdanvlviv deleted the follow-up-33637-and-33770 branch September 9, 2018 20:08
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