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

add migration table name as optional parameter #49

Closed
wants to merge 3 commits into from

Conversation

lucagrulla
Copy link

Sometimes it's useful/needed to be able to control the migrations table name.

The pull request is adding a :table option to the ragtime configuration map that will override the default name("ragtime_migrations"). If no table option is passed the migration table will still use the regular ragtime_migrations

{:migrations ragtime.sql.files/migrations
 :database "jdbc:mysql://localhost:3306/example_db?user=root"
  :table "table_name_i_like"}

Will initially create and then use a table called table_name_i_like to keep track of the applied migrations.

@weavejester
Copy link
Owner

First you'll need to get the tests running, and clean up your commit a little before it can be merged.

Your commit adds in a temporary file, .nrepl-port, and removes the line endings from several files. Your commit message should also be capitalised, and you should make sure that the commit contains only changes related to the PR, so remove the Codox dependency.

Regarding the change itself, you've updated the connection multimethod to contain a map of options, however I don't believe this is the right solution. How to connect to a database is independent of the name of the migrations table, so we shouldn't complect the database connection with migration options.

@lucagrulla
Copy link
Author

Apologies for the tests,

I fixed it but then reverted for different reason and forgot to reimplement the fix before pushing and send the pull requests.

Regarding the change and the complecting connection and migrations: I believe it's more a semantic problem in how the "connection" multimethod is named than the implementation. SqlDatabase record is not just a connection, it knows how to interact with the database(migrate forward and backward). From that perspective connection function is more a "get-database" than anything else, the actual connection is on demand(sql/with-connection) on each of the 3 functions of the Migratable protocol, not created within the connection function itself. From that perspective is therefore fine to pass all is needed to interact with the database to achieve the final business goal

So I could rename connection to something else and we then can keep the code and also have a better semantic.

If you don't see this as an issue we could explicitly assoc the option value once the Database record is available to the migrate and rollback functions in ragtime.main. Something along the way of:

(defn migrate [{:keys [migrations table] :as options}]
  (core/migrate-all
   (assoc (core/connection options) :table table)
   (resolve-migrations migrations)))


(defn rollback [{:keys [database migrations table]} & [n]]
  (let [db (assoc (core/connection database) :table table)]
    (doseq [m (resolve-migrations migrations)]
      (core/remember-migration m))
    (core/rollback-last db (or (when n (Integer/parseInt n)) 
                               1))))

What do you think?

@lucagrulla
Copy link
Author

Apologise for the long time it took but I fix finally fixed the broken test. Before moving to clean up the pull requests following your suggestions(cleaning up white spaces I inadvertently added and using capitalisation of commit messages) I'm wondering if you had a chance to think about my previous comment. Any thoughts? :-)

@weavejester
Copy link
Owner

Fixed in 0.4.0 onward.

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.

None yet

2 participants