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

Allow pre existing connections to be used #20

Conversation

nealio82
Copy link
Contributor

Depends on #19

This PR breaks the config into encapsulated classes and removes a lot of the coupling between the migrator and the App class. It allows injection of connections into the migrator, so pre-defined Connection objects can be used, rather than requiring using the config.json file. It is a first-step in tidying and abstracting some of the parts of the system, with the goal of moving toward SRP and cleaner APIs between objects.

I've also added unit tests for some of the classes I've introduced, while being careful not to modify the existing integration test suite.

@nealio82 nealio82 force-pushed the allow-pre-existing-connections-to-be-used branch from 8bdeeec to 56dbc7e Compare December 29, 2020 11:10
@nealio82 nealio82 mentioned this pull request Dec 31, 2020
@michaeljoseph
Copy link
Contributor

Hey @nealio82, could you please bring this up-to-date with master now that we've fixed the build?

@nealio82 nealio82 force-pushed the allow-pre-existing-connections-to-be-used branch from 56dbc7e to e161c00 Compare January 8, 2021 11:37
@nealio82
Copy link
Contributor Author

nealio82 commented Jan 8, 2021

hey @michaeljoseph, i rebased onto the updated master

src/Migrator/Migrator.php Outdated Show resolved Hide resolved
src/SamplerMap/SamplerMap.php Outdated Show resolved Hide resolved
tests/Collection/TableCollectionTest.php Outdated Show resolved Hide resolved
src/Collection/TableCollection.php Outdated Show resolved Hide resolved
src/Migrator.php Outdated Show resolved Hide resolved
@nealio82
Copy link
Contributor Author

@noondaysun normally I'd have batch-applied your suggestions so you get onto the authors list as well, but the change in Migrator.php needed to be done in the IDE as I needed to delete a few lines lower down too, so I did the whole lot in one go - but thanks for jumping in!

@martinmca martinmca merged commit 45938b9 into MapleSyrupGroup:master Feb 1, 2021
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.

4 participants