search

banzai-inc / fixtures-component

Development fixtures component for the reloadable pattern
Eclipse Public License 1.0
6 stars 1 forks source link

Use :db instead of :store #1

Closed dadair-ca closed 9 years ago

dadair-ca commented 9 years ago

Duct components such as Ragtime require the datastore to be identified by :db, while this component requires :store. To reduce friction for integrating with Duct, I've updated this component from :store => :db. This is also in-line with more common clojure name conventions for databases.

I've tested the changes in a local Duct project, and was able to run both migrations (using Ragtime) and fixtures (using this component), so the renaming is working.

I tried running tests, but I got the following output:

Aug 25, 2015 10:55:03 PM clojure.tools.logging$eval1448$fn__1452 invoke
WARNING: Table "NON_EXISTENT_TABLE" not found; SQL statement:
INSERT INTO non_existent_table ( id ) VALUES ( ? ) [42102-170]
Aug 25, 2015 10:55:03 PM clojure.tools.logging$eval1448$fn__1452 invoke
WARNING: Table "NON_EXISTENT_TABLE" not found; SQL statement:
DELETE FROM non_existent_table [42102-170]

Ran 3 tests containing 0 assertions.
0 failures, 0 errors.

I checked out the base repository into a new directory, and ran the migrations and the tests, and received the same output, so I am not sure what is going on with the repository's tests, but both my changes and the base repository output the same (above).

kendagriff commented 9 years ago

Thanks, I'll accept the pull request as soon as I can. The output you're referring to is logging information triggered by one of the tests - it's intentional. If migrations are part of your system startup, you can end up in a situation where fixtures are being run before you've had the chance to actually migrate the database. Therefore, I provided an outlet for a small bit of logging output to alert the user to that fact.

There's got to be a more robust way to deal with that. Any thoughts?

dadair-ca commented 9 years ago

Hmm I can give it some thought. I think for now, maybe just simplify the output?

Ex: "Couldn't find table 'non_existing_table'". Do you have migrations to run?"

Although this error might be confusing, as people may think the library itself deals with migrations.

Maybe keep it the way it is, but have a part of the README dedicated to this? It could explain that you may need to update your database, with an example of using ragtime?

dadair-ca commented 9 years ago

And since this PR would be considered a breaking change, maybe bump the major version? I bumped the minor but minors shouldn't break builds.