ash-project / igniter

A code generation and project patching framework.
https://hexdocs.pm/igniter/readme.html
MIT License
179 stars 21 forks source link

`Igniter.Project.Config.configure/6` Inserts Misformatted Config Code #104

Closed type1fool closed 3 weeks ago

type1fool commented 3 weeks ago

Describe the bug Config modifications result in string-atoms as keys when config_path is a module name. This appears to only be the case when Igniter modifies existing code, and not when a new config block is inserted.

To Reproduce

# my_igniter.install.commanded.ex
def igniter(igniter, argv) do
  app_name = Igniter.Project.Application.app_name(igniter)
  app_module = app_name |> to_string() |> Macro.camelize()
  repo = Macro.camelize("#{app_name}") |> Module.concat("Repo")

  # these two variables are atoms when passed to IO.inspect/1:

  event_store_module =
    app_name
    |> to_string()
    |> Macro.camelize()
    |> Module.concat("EventSystem.EventStore")

  event_system_application_module =
    app_name
    |> to_string()
    |> Macro.camelize()
    |> Module.concat("EventSystem.Application")

  igniter
  |> Igniter.compose_task("cauldron.install.ecto", [])
  |> Igniter.Project.Deps.add_dep({:commanded, "~> 1.4"})
  |> Igniter.Project.Deps.add_dep({:commanded_ecto_projections, "~> 1.4"})
  |> Igniter.Project.Deps.add_dep({:commanded_eventstore_adapter, "~> 1.4"})
  |> Igniter.apply_and_fetch_dependencies(yes: true)
  |> Igniter.Project.Config.configure("config.exs", app_name, :consistency, :eventual)
  |> Igniter.Project.Config.configure("config.exs", app_name, :event_stores, [
    event_store_module
  ])
  |> Igniter.Project.Config.configure(
    "config.exs",
    app_name,
    event_store_module,
    migration_primary_key: [type: :binary_id],
    migration_foreign_key: [type: :binary_id],
    migration_timestamps: [type: :utc_datetime_usec]
  )
  |> Igniter.Project.Config.configure(
    "config.exs",
    app_name,
    event_system_application_module,
    event_store: [
      adapter: Commanded.EventStore.Adapters.EventStore,
      event_store: event_store_module
    ],
    phoenix_pubsub: [
      adapter: Phoenix.PubSub.PG2,
      pool_size: 1
    ],
    pubsub: :local,
    registry: :local,
    snapshotting: %{}
  )
end

Expected behavior

# config.exs
config :yolo,
  ecto_repos: [Yolo.Repo],
  generators: [timestamp_type: :utc_datetime],
  consistency: :eventual,
  event_stores: [Yolo.EventSystem.EventStore]

config :yolo, Yolo.EventSystem.EventStore,
    migration_primary_key: [type: :binary_id],
    migration_foreign_key: [type: :binary_id],
    migration_timestamps: [type: :utc_datetime_usec]

config :yolo, Yolo.EventSystem.Application,
    event_store: [
      adapter: Commanded.EventStore.Adapters.EventStore,
      event_store: Yolo.EventSystem.EventStore
    ],
    phoenix_pubsub: [adapter: Phoenix.PubSub.PG2, pool_size: 1],
    pubsub: :local,
    registry: :local,
    snapshotting: %{}

Actual behavior

The following :yolo config was modified - only :ecto_repos & :generators existed before running the Igniter task.

# config.exs
config :yolo,
  ecto_repos: [Yolo.Repo],
  generators: [timestamp_type: :utc_datetime],
  consistency: :eventual,
  event_stores: [Yolo.EventSystem.EventStore],
  "Elixir.Yolo.EventSystem.EventStore": [
    migration_primary_key: [type: :binary_id],
    migration_foreign_key: [type: :binary_id],
    migration_timestamps: [type: :utc_datetime_usec]
  ],
  "Elixir.Yolo.EventSystem.Application": [
    event_store: [
      adapter: Commanded.EventStore.Adapters.EventStore,
      event_store: Yolo.EventSystem.EventStore
    ],
    phoenix_pubsub: [adapter: Phoenix.PubSub.PG2, pool_size: 1],
    pubsub: :local,
    registry: :local,
    snapshotting: %{}
  ]

This config was added by the Igniter task, and this key did not exist previously.

# runtime.exs
  config :yolo,
         Yolo.EventSystem.EventStore,
         url:
           System.get_env("DATABASE_URL", "ecto://postgres:postgres@localhost/yolo_eventstore"),
         database: "yolo_eventstore",
         pool_size: System.get_env("POOL_SIZE", "10") |> String.to_integer(),
         socket_options:
           if(System.get_env("ECTO_IPV6") in ~w(true TRUE 1), do: [:inet6], else: [])

** Runtime

Additional context

In addition to resolving the bug, it may be useful to add an option to Config.configure/6 to force a new config block for the given app_name and config_path.

zachdaniel commented 3 weeks ago

In addition to resolving the bug, it may be useful to add an option to Config.configure/6 to force a new config block for the given app_name and config_path.

This would be interesting, but there is some complexity there, like how to make sure that subsequent calls to config will actually use that block. We want to try to honor whatever existing configuration patterns the user has in their config files, so I hesitate to set up the pattern that every generator always ends up appending to config even if the user has already configured some aspect of the library/app...not sure, needs more thought.

type1fool commented 3 weeks ago

With the main branch Igniter, I am seeing correctly formatted atoms. 🎉 The indentation is still a bit unusual, but that's no big deal. Thank you!

zachdaniel commented 3 weeks ago

Yeah, I think we can encode some "preference" there with some sourceror metadata? Its not super easy to manage that formatting though.