Configurable migrations path #29
Conversation
| @@ -16,6 +16,17 @@ defmodule ExcellentMigrations.CredoCheck.MigrationsSafetyTest do | |||
| |> assert_issues() | |||
| end | |||
|
|
|||
| test "it should report code that is defined in user specified directory" do | |||
| Application.put_env(:excellent_migrations, :migrations_paths, [ | |||
There was a problem hiding this comment.
Using Application.put_env requires restoring the original value after the test is finished (example). Otherwise the new value will stay there and will affect other tests. Maybe the problem isn't manifested in any way now, but in the future that might change.
Also because it means editing global config, it may no longer be possible to run test asynchronously. I don't recommend the approach with Application.put_env. Instead I propose:
- Passing
migrations_pathstoFilesFinder.get_migrations_pathsas an argument. This way the test won't need to set the config. - Or wrapping interaction with config in a new module with interface and create mock for tests with different implementation.
I'd go with the first approach – I believe it's fine & simple.
| @@ -10,9 +10,12 @@ defmodule ExcellentMigrations.FilesFinder do | |||
| end | |||
|
|
|||
| def relevant_file?(path, start_after) do | |||
| migrations_paths = | |||
| Application.get_env(:excellent_migrations, :migrations_paths, ["migrations/"]) | |||
There was a problem hiding this comment.
I like that a list of user defined paths is passed here (not just a single one) 👍🏻
|
@kraleppa Also, if you rebase on fresh master, CI checks should now work. |
|
This is a nice improvement, I would love to see something like this get merged. |
|
@Artur-Sulej Can I help get this merged? I can create a new PR with the required changes if that's necessary. |
Decided to solve this issue 😄 #6
Let me know if something should be fixed here