Skip to content

Conversation

@lread
Copy link
Contributor

@lread lread commented May 4, 2025

Closes #162
Closes #164

@lread
Copy link
Contributor Author

lread commented May 4, 2025

Up for debate:

  1. Where to put the new code. I think it fits where I put it but you might have other ideas! Feedback welcome.

  2. Not supporting mixed types for :up :down, ex:

    {:id "010-bad"
     :up some-ns/some-sym
     :down ["DROP TABLE bar"]}
  3. No extra user-friendly error checks.
    The above won't work but we don't explicitly check and present a
    user-friendly error.

  4. Failing at migration application time if clj migration fn does not resolve.
    Could try to resolve after load and fail earlier.

  5. In the issue there was the idea of adding an option to enable clj migrations (off by default).
    Still want/need?

  6. I don't think there is any special work needed for transactions?
    The clojure up down code can decide to use transactions or not.

@weavejester
Copy link
Owner

Thanks for the PR! To answer your questions:

  1. Where you put the code is fine.
  2. Mixed types should be supported. Rather than using a separate CljMigration record, you could just put the symbol resolution in SqlMigration.
  3. This problem goes away if we support symbols in both :up and :down keys.
  4. Failing early sounds like a good idea. The symbol could be resolved on load and passed to SqlMigration as a resolved function.
  5. I think we can leave this out for now. We can always put it in later by adding a verification step to the load.
  6. I think I agree with this; the function can use a transaction or not.

@lread
Copy link
Contributor Author

lread commented May 5, 2025

Thanks for the review!

2- That was my original inclination, so great!
4- Will do

I'll also address the lint warning I introduced.

@lread
Copy link
Contributor Author

lread commented May 5, 2025

Ok @weavejester, I think I am ready for another review.
Happy to tweak if you notice anything new.

@lread
Copy link
Contributor Author

lread commented May 11, 2025

When you find some time @weavejester, I'd be happy to hear what you think.

@lread
Copy link
Contributor Author

lread commented Jul 22, 2025

Hi @weavejester! I am still happy to help bring this one to completion, if you find some time/interest.

@weavejester
Copy link
Owner

Apologies for the delay, this looks okay. Can you ensure any lines you added are 80 characters or less in length, and then squash your commits down. Then it should be good to merge.

@lread
Copy link
Contributor Author

lread commented Jul 22, 2025

Thanks, @weavejester. I've addressed the 80-character line length feedback for changed lines.

I will squash commits for you after I see CI passing for this commit.

@lread
Copy link
Contributor Author

lread commented Jul 24, 2025

@weavejester, if you approve this workflow, we can witness it passing, then I can squash my commits as requested.

@weavejester
Copy link
Owner

Ah, sorry, the workflow button wasn't showing up earlier, so I assumed it had already run.

@lread lread force-pushed the lread/clj-migrations branch from ecc4f04 to 23a8e24 Compare July 24, 2025 14:37
@lread
Copy link
Contributor Author

lread commented Jul 24, 2025

No problem, @weavejester. Commits are now squashed.

@weavejester weavejester merged commit bd0a1e4 into weavejester:master Aug 1, 2025
1 check passed
@weavejester
Copy link
Owner

Thanks for the PR and apologies for the delays in getting it merged.

@lread
Copy link
Contributor Author

lread commented Aug 27, 2025

Hi @weavejester, what do you think about cutting a release?

@weavejester
Copy link
Owner

0.12.0 released. Apologies for the delay.

@lread
Copy link
Contributor Author

lread commented Sep 4, 2025

Thanks @weavejester!

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.

Did refactoring break custom Migration? Deletion of ragtime.jdbc/load-files breaks ragtime-clj

2 participants