Open ibarakaiev opened 1 month ago
🤔 I think that the argument order there is weird, as you've said. However, the two functions you're proposing Config.configures_root_key?/2
and Config.configures_key?/3
don't fully cover the case. We use that frequently in Ash to check if some nested configuration is set.
I think that what we should really do is modify Config.configures?/3
to handle the additional cases you've identified.
There is a version of configures?
that allows for providing the file currently.
@spec configures?(Igniter.t(), file :: String.t(), list(atom), atom()) :: boolean()
def configures?(igniter, file, path, app_name) do
file_path = Path.join("config", file)
igniter =
Igniter.include_existing_elixir_file(igniter, file_path, required?: false)
case Rewrite.source(igniter.rewrite, file_path) do
{:ok, source} ->
source
|> Rewrite.Source.get(:quoted)
|> Zipper.zip()
|> configures?(path, app_name)
_ ->
false
end
end
As for configuration's being listable twice, that is pretty tough. We basically need to start from the bottom and work our way up or recurse. It's not ideal. I think the idea of configuring a given value twice in the same scope can potentially be ignored for now?
Hm, found this reference and I get what you mean now: https://github.com/ash-project/ash_postgres/blob/4cb3a72c54d42ead9619672f538d5ceab8416a08/lib/mix/tasks/ash_postgres.install.ex#L59. However, I do find the current implementation of configures?/3
accepting a list problematic because realistically the list shouldn't ever be more than two atoms long, should it? So really there are only two cases we need to account for, something like:
configures_key?(igniter, "config.exs", :otp_app, :key)
# or
configures_key?(igniter, "config.exs", :otp_app, Repo, :key)
We can modify the above proposal for configures_key?
to have two arities then.
We basically need to start from the bottom and work our way up or recurse.
Or at the top and recurse down, since we don't need to know the actual value, just that it is set.
And if we ever want to check if just the root_key
is configured (i.e. you need to check if config :sentry
exists but don't care about its settings), then configures?/3
would need to accept an empty list.
I don't think it's problematic, it a very standard pattern to have nested configuration in keyword lists and so we want to support that. Using the list to support it makes sense to me.
Accepting an empty list and/or adding a configures_app?
(the first key of config should be an application name) would make sense though, as you are right there isn't currently a way to ask if just the top level configuration is set.
And yeah we don't need to change anything for configures?
in terms of going bottom-up/top down. It already finds any config in the file that matches the given arguments. The thing I was thinking of is if you had the ability to ask what a specific value is configured to, you'd need to start at the bottom and go up, so as to get the last one specified in the file. But we don't have that feature so it doesn't really matter :)
Will send a PR shortly :)
Facing some issues with retrieving configuration in Igniter. Specifically,
Igniter.Project.Config.configures?/3
seems to return:error
where it shouldn't. I can make a PR for it to returnfalse
but I think there are a few more issues, outlined below.There are two versions of
Config.config/2,3
, one that accepts just theroot_key
andopts
and one that also accepts a givenkey
for theroot_key
: https://hexdocs.pm/elixir/main/Config.html#config/2. Tbh I'm not entirely sure how they're different since one can achieve the same functionality with both:I guess it's a convenience and a convention to only define configuration up to two nested levels deep. This means that
Igniter.Project.Config.configures/3
needs to search for eitherConfig.config/2
orConfig.config/3
. In principle, it is easy to assume that one needs to search forConfig.config/2
if the providedconfig_path
is a single element, andConfig.config/3
when it has more than one element. But in realityConfig.config/2
may be nested too (like in the example above), meaning that when there's more than one element,Config.configures/3
needs to search for both versions of theconfig
. Right now what happens isConfig.configures/3
searches forConfig.config/3
whenconfig_path
is one element, and forConfig.config/2
whenconfig_path
is more than one element. This seems incorrect, and at least in my case it doesn't quite work.One additional complication that needs to be accounted for is that a certain key may be configured twice, like in the code examples in the official docs linked above. Therefore, a certain nested key may not be configured in the first
Config.config/2,3
match but may be configured in a later one. So there needs to be some kind of amove_right()
+ recursion if there was no nested match in a givenConfig.config/2,3
match.Finally, I found the function signature of
Config.configures?/3
a bit tricky to reason about since theapp_name
is last in the list, whereas inconfig/2,3
it is first and is calledroot_key
.Considering the above, I wanted to propose the following change:
Config.configures_root_key?(zipper, root_key)
which will check if eitherconfig :my_app, _
orconfig :my_app, _, _
is present.Config.configures_key?(zipper, root_key, key)
which will check if eitherconfig :my_app, key: _
orconfig :my_app, :key, _
is present.config/2
andconfig/3
are exposed andApplication.get_env/3
takes only one atom), we don't recursively check for keys. That will be considered an advanced use case and should be manually handled with a Zipper when needed.Config.configures?/3
in favor of the above.