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

fix: preserve original ordering in Util.Install #33

Merged

Conversation

ibarakaiev
Copy link
Contributor

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

Depending on the order in which Mix loads tasks, the original installer ordering may be lost. For example, mix igniter.install ash_postgres@path:../ash_postgres,ash_money@path:../ash_money installs AshMoney before AshPostgres. This PR adds a sorting step to return the original ordering passed as arguments to igniter.install.

|> Enum.sort_by(
&Enum.find_index(desired_tasks, fn e -> e == Mix.Task.task_name(&1) end),
&<=/2
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a cleaner approach here would be something like:

   available_igniter_tasks =
      Mix.Task.load_all()
      |> Stream.map(fn item ->
        Code.ensure_compiled!(item)
        item
       end)
       |> Stream.filter(&implements_behaviour?(&1, Igniter.Mix.Task))
       |> Enum.filter(&(Mix.Task.task_name(&1) in desired_tasks))

igniter_tasks = Enum.filter(desired_tasks, &(&1 in available_igniter_tasks))

? Maybe not though, they kind of both aren't 100% clear what's going on. I'll leave it up to you just LMK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is tricky because desired_tasks would need to be converted from strings to modules before checking for in available_igniter_tasks. And the conversion is not deterministic, which is why we have load_all() |> map() |> filter(), right?

@zachdaniel zachdaniel merged commit df409b0 into ash-project:main Jul 4, 2024
15 checks passed
@zachdaniel
Copy link
Contributor

🚀 Thank you for your contribution! 🚀

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.

2 participants