jeremyjh / dialyxir

Mix tasks to simplify use of Dialyzer in Elixir projects.
Apache License 2.0
1.7k stars 140 forks source link

Do not load optional applications #509

Closed aptinio closed 1 year ago

aptinio commented 1 year ago

Thanks to @josevalim's explanation: https://elixirforum.com/t/elixir-v1-15-0-rc-0-released/56019/27

Fixes #502.

josevalim commented 1 year ago

Unfortunately this is not correct. You don't want to delete all optional applications, you still want to load them. Instead, you want to not report them in case they cannot load.

aptinio commented 1 year ago

@josevalim, you got me thinking... so I tested it out to make sure. The computed deps_apps in the project I'm working on are the same on Elixir 1.14 without this PR and on Elixir 1.15 with this PR.

Since deps_apps/2 is being called recursively, the optional deps that are explicitly installed still get included in the first call to deps_apps.

Also, as far as I can tell, this PR is replicating the old behavior that you mentioned in https://elixirforum.com/t/elixir-v1-15-0-rc-0-released/56019/27:

In earlier Elixir versions, foo.app would look like this if gettext was not added as dependency somewhere:

{applications, []}

But now it looks like this:

{optional_applications, [gettext]}
{applications, [gettext]}

What do you think? Thanks for looking into this! :relaxed:

josevalim commented 1 year ago

This will be true because you don't use gettext. But someone may use gettext, and now it will be removed, while it should have been listed.

GRoguelon commented 1 year ago

@josevalim I made a proposal on @aptinio 's repo: https://github.com/aptinio/dialyxir/pull/1

GRoguelon commented 1 year ago

Hi @jeremyjh,

I've submitted a PR for the issue related to Unknown application with optional applications. Should I create a new PR directly in this repo?

Thank you.

jeremyjh commented 1 year ago

@GRoguelon yes please make a new PR on this repo and we'll close this one.

GRoguelon commented 1 year ago

@jeremyjh Done: #514