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.

Sign up for GitHub

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: find app_name when kw list is defined at the end #236

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: find app_name when kw list is defined at the end #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/igniter/project/application.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ defmodule Igniter.Project.Application do
|> Sourceror.Zipper.zip()

with {:ok, zipper} <- Igniter.Code.Function.move_to_def(zipper, :project, 0),
zipper <- Igniter.Code.Common.rightmost(zipper),
true <- Igniter.Code.List.list?(zipper),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keyword.get_key/2 does check if it's a list already.

{:ok, zipper} <-
Igniter.Code.Common.move_to(zipper, &match?({:__block__, _, [:app]}, &1.node)),
Copy link
Contributor

@zachdaniel zachdaniel Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this might be too liberal. Any other keyword list that contains an app option, anywhere in the structure could be construed as the app config we are looking for, no matter how deep we search. I think we should start simpler, going to the rightmost literal list, and otherwise complaining. We have a deps_location pattern for allowing users to configure where in the deps function their list is, so we could eventually do similar patterns here, but this method can yield false positives which I think is more dangerous.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was busy when I typed the above, sorry 😆 its not very clear.

The current feature could have a false positive, i.e if someone had something like this:

def project() do
  [
    foo: [app: :foo], # <- we'd find this one.
   app: :foo
  ]
end

And false positives are dangerous. I think "left most list" is really the only safe heuristic to apply for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was clear enough 😄

Yeah that makes sense, I'm gonna look at that deps_location and soon push changes to this branch.

zipper = Zipper.up(zipper),
zipper = Zipper.up(zipper),
{:ok, zipper} <- Igniter.Code.Keyword.get_key(zipper, :app),
{:ok, app_name} when is_atom(app_name) <- expand_key_value(zipper) do
app_name
Expand Down
50 changes: 50 additions & 0 deletions test/igniter/project/application_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,56 @@ defmodule Igniter.Project.ApplicationTest do
assert Igniter.Project.Application.app_name(igniter) == :igniter_test
end

test "it returns the application name when keyword list is defined at the end" do
igniter =
test_project(
files: %{
"mix.exs" => """
defmodule IgniterTest.MixProject do
use Mix.Project

def project do
releases = [
test: []
]

[
app: :igniter_test,
releases: releases
]
end
end
"""
}
)

assert Igniter.Project.Application.app_name(igniter) == :igniter_test
end

test "it returns the application name when keyword list is assigned to a variable" do
igniter =
test_project(
files: %{
"mix.exs" => """
defmodule IgniterTest.MixProject do
use Mix.Project

def project do
project = [
app: :igniter_test,
releases: releases
]

project
end
end
"""
}
)

assert Igniter.Project.Application.app_name(igniter) == :igniter_test
end

test "it raises if the application name can't be resolved" do
igniter =
test_project(
Expand Down
Loading