Proposal: Searching for implementations with keys

[NickNeck]

Hi, Sascha

my suggestion would be to extend the config_key option so that a list is also accepted.
That would be usefull for behaviours with additional configurations.
Example:

config :my_app, :sys,
  module: MyApp.Sys.Impl
  timeout: 2_000

defmodule MyApp.Sys do
  use Knigge,
    otp_app: :my_app,
    config_key: [:sys, :module]
  ...
end

What do you think?

Kind regards, Marcus

p.s. Nice work and I love the name of the lib.

[sascha-wolf]

Sadly, this doesn't really play nice with the default changes from #18.

It would also fail quietly when key exists but get_in returns nil for keys.

Instead I'd would expect to receive an error along the lines of:

Unable to fetch implementation for MyFacade from config of app <otp_app> under path <keys>.

The message could be improved upon but I think you catch my drift?

[sascha-wolf]

Instead of introducing :keys I'd argue it's a good opportunity to introduce an "or" mechanism:

config_key: [:atom, {:list_of, :atom}]

Where valid_value?/2 would look like this:

defp valid_value?({:list_of, type}, value),
  do: is_list(value) and Enum.all?(value, &valid_value?(type, &1))

defp valid_value?(list, value) when is_list(list),
  do: Enum.any?(list, &valid_value?(&1, value))

In addition the transform function could then be extended to map config_key to always be a list (by using List.wrap/1).

Which in turn would simplify the implementation in Implementation.fetch!/1 as there no need for multiple clauses anymore (one for single atom, one for list).

[coveralls]

Coverage increased (+0.8%) to 95.652% when pulling 66a7cfb on hrzndhrn:proposal/config_key into e0886d3 on sascha-wolf:main.

[escobera]

Hey @sascha-wolf what do you think is needed to merge this? I'm available to help if needed. Could really use this feature =)

[sascha-wolf]

Apart from some cosmetic suggestions and one small concern, LGTM 💙💜❤️🧡💛💚

[sascha-wolf]

🎨 If it's okay I'd prefer to write it like this:

Suggested change

	def fetch!(%Options{implementation: {:config, otp_app, [key | keys]}, default: default}) do
	with nil <- otp_app |> env!(key, default) |> get(keys) do
	raise ArgumentError,
	message: """
	could not fetch application environment #{inspect([key | keys])} \
	for application #{inspect(otp_app)}\
	"""
	def fetch!(%Options{implementation: {:config, otp_app, [key | keys]}, default: default}) do
	otp_app
	|> env!(key, default)
	|> get(keys)
	|> case do
	nil ->
	raise ArgumentError,
	"could not fetch application environment #{inspect([key | keys])} for application #{inspect(otp_app)}"
	implementation ->
	implementation
	end

[sascha-wolf]

⚠️ What's the train of thought here of swapping these two? This would actually break the delegate_at_runtime? default, since the "environment" spec gets resolved to a boolean in transform/2.

[sascha-wolf]

🧹 I think we can drop this now, right?

Suggested change

	defp valid_value?(:keys, value),
	do: is_atom(value) || (is_list(value) && Enum.all?(value, &is_atom/1))

[NickNeck]

Hi, Sascha

I have made some more changes and incorporated your change requests.

I have also updated the deps and the ci.yml.

Because use Mix.Config is deprecated I have changed it to import Config. This causes the failing tests for Elixir versions smaller as 1.9.0.

Let me know if the PR is too big and how we want handle the failing tests.

Have a nice day.

[sascha-wolf]

Hey @NickNeck, I've to say the PR currently intimidates me. I'm also not sure why you updated the ci.yml flow?

If you think the project can derive value from the changes to ci.yml and config/ then let's extract them to a different PR so we can keep the focus on the originally proposed changes. Otherwise I don't feel comfortable approving and merging this.

[sascha-wolf]

Reading this test, I think it'd be beneficial for readability to deprecate config_key in favor of config_path. The Options module has a map_deprecated function (link) which we can use to map config_key to config_path while also printing a deprecation warning.

What do you think?

[NickNeck]

Good idea. I will add this.

[NickNeck]

I just update the ci.yml to test against the latest versions. I used my little git_hub_actions tool to make things simpler for me. I will make a separate PR and then you can take look if it is okay for you.

[SukhikhN]

Hello @NickNeck and @sascha-wolf!
Would you be so kind as to bring this PR to a release?

Now I fetch an implementation module from nested configuration key and pass it to Knigge directly with the :implementation option, but with this approach I can't change it in runtime.

[sascha-wolf]

Hello @NickNeck and @sascha-wolf! Would you be so kind as to bring this PR to a release?

I'll try to find some time this week. Life has not been kind lately.