search

jeremyjh / dialyxir

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

Improve handling of local dependencies #401

Open josevalim opened 4 years ago

josevalim commented 4 years ago

Treat all path dependencies as files in the current project instead of remote dependencies. This simplifies the logic as we no longer need to especially handle umbrella applications, as they are all path dependencies anyway.

This requires Elixir v1.10+.

Also note this patch completely removes :plt_add_deps because it did not work before. Since the Plt.check/2 function always adds all transitive dependencies, any attempt to discard deps by setting :plt_add_deps to :apps_deps had no effect. This PR does not remove :apps_deps from the documentation but I would recommend so to be done.

josevalim commented 4 years ago

Note this PR has one issue. If A depends on B as a path dependency, both B and A are verified when running mix dialyzer from A. That's how Mix treats path dependencies (as if they were part of the same project), but it now means you may get warnings from B directly in A.

There is one obvious solution to this problem, which is to automatically ignore all warnings from B when running the command from A, but I haven't implemented that because I am not sure if it is desired or not.

jeremyjh commented 4 years ago

Note this PR has one issue. If A depends on B as a path dependency, both B and A are verified when running mix dialyzer from A. That's how Mix treats path dependencies (as if they were part of the same project), but it now means you may get warnings from B directly in A.

I believe that is how it has always worked because we'd actually run from the root in an Umbrella project, though it has been a requested change in the past to limit warnings to the current child. This PR makes that much more feasible but we can add an issue for it rather than address it in this PR.

On another note, if this requires Elixir 1.10 should we add elixir: "elixir: ">= 1.10.0" in the project key? I'm not totally clear if this is used by hex in version resolution.

josevalim commented 4 years ago

On another note, if this requires Elixir 1.10 should we add elixir: "elixir: ">= 1.10.0" in the project key? I'm not totally clear if this is used by hex in version resolution.

We should. It is not used by Hex but you should update it. I didn't do the change because it most likely requires changing other places too, such as CI configs and so on. :) Perhaps we should do the change on master first before merging this PR?

jeremyjh commented 4 years ago

Ok, I'll do one more release from master and then update configs and README for a new minimum Elixir version before merging this.

Nezteb commented 1 year ago

FYI I pulled this and did a rebase of upstream master and everything seems to still work as expected. 😄

jeremyjh commented 1 year ago

Yeah, I wish I'd documented this at the time but I ran into a doubt regarding these changes; from what I recall I found that plt_add_deps did work, but I'm not sure I'd proved that to my satisfaction and hence didn't object to it, but wasn't ready to merge it either. I meant to take another look at it, and then three years passed. Thanks for putting it back on my radar, but all I can really say is I have to give another serious look.