Add :migrate_unless option to formatter #13844
Conversation
| * `:migrate_unless` (since v1.18.0) - when `true`, | ||
| rewrites `unless` expressions using `if` with a negated condition, for example | ||
| `if foo, do:` becomes `unless !foo, do:`. | ||
| Defaults to the value of the `:migrate` option. This option changes the AST. |
There was a problem hiding this comment.
What about the corner cases we discussed in the past, such as unless x = expr() do, do we handle those as well? Or we don't? If we don't, we must say not all cases can be automatically migrated. :)
There was a problem hiding this comment.
We do handle these, it becomes if !(x = expr()) which is terrible code, but it does work fine and I expect unless x = expr() to be a very rare need anyway since x could only be false or nil. WDYT?
Co-authored-by: Juan Peri <eternoperegrino@gmail.com>
|
Shipped this in styler's 1.1 today - though I see I'm a weekend behind elixir-lang! I ended up using Anyways, thanks for taking care of this in core ❤️ Looking forward to just passing |
| defp negate_condition(condition) do | ||
| case condition do | ||
| {neg, _, [condition]} when neg in [:!, :not] -> condition | ||
| {:|>, _, _} -> {:|>, [], [condition, {{:., [], [Kernel, :!]}, [closing: []], []}]} |
There was a problem hiding this comment.
we found the migrations more readable not doing this particular transformation, as this hides the most impactful part of the statement. compare:
# given:
unless Widget |> where(foo: ^bar) |> Repo.exists?(), do: ...
# rewritten to if with pipe
if Widget |> where(foo: ^bar) |> Repo.exists?() |> Kernel.!(), do: ...
# treated as any other statement
if !(Widget |> where(foo: ^bar) |> Repo.exists?()), do:
our team asked for the latter as it has a much more natural-language read (not to mention that Kernel.!() is unlikely to be written by a human in any case)
There was a problem hiding this comment.
Thank you for the suggestion 💜 Even if I'm the one who originally implemented it as |> Kernel.!(), now that you mention it I have to agree.
Plus this is removing complexity so I'm all for it.
Do you want to send a PR @novaugust?
There was a problem hiding this comment.
happy to, i'll kick one out tomorrow
Take 2 for #13769.
The prep work has been done in: