Add override_for to dep options #14156
Conversation
override_for gives more information about when deps should explicitly be overridden or not. If a dep no longer needs an override, a message will be written indicating so. Alternatively, if a dep needs an override when a dep is already using override_for, it will alert the dev. With override: true set, if a user were to add a new dep that depends on the overridden app, there would be no indication of a potential issue.
|
An attempt at implementing #14082 I'm not familiar enough to get override_for: to fail in the same way as a missing override would, though I'm not sure if that is desired based on the conversations in the issue. I primarily tried to only add messaging in the Will update docs and polish the work up if it looks like the right approach! |
| @@ -299,7 +299,7 @@ defmodule Mix.Dep.Converger do | |||
| end | |||
|
|
|||
| cond do | |||
| in_upper? && other_opts[:override] -> | |||
| in_upper? && (other_opts[:override] || override_for?(other_opts, app)) -> | |||
There was a problem hiding this comment.
I was most confused in this section of the converge logic.
If a dep passes this check once, does it never fail/process again? I initially had a test where I wanted dependency resolution to fail if override_for: [:foo] is specified, but :baz also required the override.
| @@ -0,0 +1,11 @@ | |||
| defmodule OverrideForRepo do | |||
There was a problem hiding this comment.
Instead of adding a new fixture, could we use one of the existing ones? In particular, some fixtures allows you to configure them dynamically by passing some options.
There was a problem hiding this comment.
I tried to find a fixture that would let me configure it, but I'm only seeing opts as configurable, and not the actual requirement for the dependencies themselves to force an override requirement.
I ended up just having override_for: [] and using git_repo and deps_repo like most of the other tests currently do.
|
Thank you for the PR! ❤️ To align expectations, my priorities for Elixir is:
So it make take a while (e.g. weeks). The code here is also complex, so it may be the best approach for me to validate this is by actually implementing it too, so I may end-up reusing tests mostly if our implementations diverge considerably. |
override_for gives more information about when deps should explicitly be overridden or not.
If a dep no longer needs an override, a message will be written indicating so. Alternatively, if a dep needs an override when a dep is already using override_for, it will alert the dev.
With override: true set, if a user were to add a new dep that depends on the overridden app, there would be no indication of a potential issue.