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

chore: Let db adapter to decide how to handle database target and source names. #9

Closed
wants to merge 1 commit into from

Conversation

nashby
Copy link
Contributor

@nashby nashby commented Mar 13, 2024

When I started looking into adding sqlite support I noticed that for sqlite in database.yml you have db like db/development.sqlite3 so just adding a suffix won't work, it should have a bit different logic. So I've extracted these db name method to db adapter class and future sqlite adapter can have specific logic for it.

Copy link
Owner

@theodorton theodorton left a comment

Choose a reason for hiding this comment

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

This shows I don't have tests for the Clean command, which also uses PGCluster and this change will break that command.

PGCluster (and any future database adapters) shouldn't be aware of source or target database at the instance level - the database names should be passed as method arguments.

@theodorton
Copy link
Owner

@nashby I've added tests and took the liberty of rebasing.

@theodorton
Copy link
Owner

Aaand it doesn't break 😄 I'm still a bit hesitant about this - would like to see how it would work once there is a SQLite adapter.

@theodorton
Copy link
Owner

@nashby Most, if not all, of the necessary abstractions should now be covered by #11.

@theodorton theodorton closed this Mar 16, 2024
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