CouncilDataProject / cdptools_v2

Tools you can use to interact with and run Council Data Project instances.
Other
7 stars 9 forks source link

feature/wipe-target-database-before-cloning #97

Closed isaacna closed 4 years ago

isaacna commented 4 years ago

Pull request recommendations:

isaacna commented 4 years ago

This looks incredible and it was awesome to get another PR on the back-end tooling! :)

I have some general thoughts and would love your opinion on if they should go in this PR or in a future PR.

My biggest wish, if you are up for it is to make this even more general than Cloud Firestore. CloudFirestoreDatabase inherits from Database and it would be great to require that all databases have one additional property and one additional function.

If you could add a property to the base class that's like Database.tables so that instead of having this bin script be specific to CloudFirestoreDatabase it can be reused by whichever database the cdp instance uses. Having the property means you could replace the:

target_db_tables = [coll._path[0] for coll in target_db._root.collections()]

with just:

target_db.tables

The other thing would be to add the wipe_table function to the Database base class as well so that it is required by all databases.

Thinking about this after I have typed this all out, this is a lot that should be moved to it's own PR. I will copy paste this all into a GitHub Issue.

Yeah I think this is a good idea for scalability! I can just keep it in this pr. By making the bin script reusable, you mean it should work regardless of what database instance they create right? I can do that but the user would still have to swap the instance of one of the concrete Database subclasses such as CloudFirestoreDatabase`.

isaacna commented 4 years ago

I made an new commit abstracting the the _tables property and wipe_table method. The bin script still uses CloudFirestoreDatabase but the logic outside of instantiating a Database instance should be easily reusable now. Also, let me know if I merged in my other pr from master incorrectly. I got trapped in git merge hell but I think the branch should be good now.

evamaxfield commented 4 years ago

Also, I will be updating the test configuration soon (this weekend) so that your tests actually run finally 🙃.

Thanks for all your contributions already! 🎉