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

Refactor to fix order of migration generation #61

Closed
wants to merge 2 commits into from

Conversation

suranyami
Copy link
Contributor

@suranyami suranyami commented Nov 28, 2023

Overview

Resolves #60

Generate migrations in specific order.

Changes

  • Define order of migrations
  • Iterate on an ordered list, rather than a map.

Tests

Could not work out a way to test this in-repo, but can generate an example app using the new/old wac.install to demonstrate.

Collaborators

  1. @type1fool
  2. @suranyami

Copy link
Collaborator

@type1fool type1fool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @suranyami! 👋🏻

Thank you again for reporting this issue and opening a PR to get it resolved. Your changes would fix the migration order issue, but I think there's a slightly simpler solution.

The root problem is that Elixir's map keys are unsorted, so their order is unreliable. Knowing that you experienced migration errors due to out-of-order operations, I think the ideal solution is to use a keyword list:

@template_files [
  users: "users.exs",
  user_keys: "user_keys.exs",
  user_tokens: "user_tokens.exs"
]

@templates Keyword.keys(@template_files)

This small change would ensure the migrations are generated in the correct order since keyword lists retain their order.

If you can make this adjustment before 12/9, I'll approve and merge. Otherwise, I may make the commit and add you as co-author.

@type1fool
Copy link
Collaborator

I went ahead and opened PR #62 to resolve this issue using the Keyword list solution, and added you as a co-author. I did try to pull your fork and apply a new commit, but I think it would require your commits to live in a non-main branch.

Thank you for reporting this issue and opening the PR! 💾

@type1fool type1fool closed this Dec 2, 2023
@suranyami
Copy link
Contributor Author

Hi.

Thanks, I like your solution even better. Well done.

Sorry I didn't respond earlier, was getting a flood of noise on a couple of repos I was watching and have since muted them.

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.

[BUG] - migrations created non-deterministically
2 participants