-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add option to formatter to rewrite unless #13769
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.
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
Changes from all commits
5a1b5c5
fe87314
4a3cc20
a8aba2a
7855b06
9213315
3d2dbff
2162ad7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,6 +191,7 @@ defmodule Code.Formatter do | |
sigils = Keyword.get(opts, :sigils, []) | ||
normalize_bitstring_modifiers = Keyword.get(opts, :normalize_bitstring_modifiers, true) | ||
normalize_charlists_as_sigils = Keyword.get(opts, :normalize_charlists_as_sigils, true) | ||
rewrite_unless = Keyword.get(opts, :rewrite_unless, false) | ||
syntax_colors = Keyword.get(opts, :syntax_colors, []) | ||
|
||
sigils = | ||
|
@@ -217,6 +218,7 @@ defmodule Code.Formatter do | |
file: file, | ||
normalize_bitstring_modifiers: normalize_bitstring_modifiers, | ||
normalize_charlists_as_sigils: normalize_charlists_as_sigils, | ||
rewrite_unless: rewrite_unless, | ||
inspect_opts: %Inspect.Opts{syntax_colors: syntax_colors} | ||
} | ||
end | ||
|
@@ -484,6 +486,27 @@ defmodule Code.Formatter do | |
binary_op_to_algebra(:in, "not in", meta, left, right, context, state) | ||
end | ||
|
||
# rewrite unless as if! | ||
defp quoted_to_algebra( | ||
{:unless, meta, [condition, block]}, | ||
context, | ||
%{rewrite_unless: true} = state | ||
) do | ||
quoted_to_algebra({:if, meta, [negate_condition(condition), block]}, context, state) | ||
end | ||
|
||
defp quoted_to_algebra( | ||
{:|>, meta1, [condition, {:unless, meta2, [block]}]}, | ||
context, | ||
%{rewrite_unless: true} = state | ||
) do | ||
quoted_to_algebra( | ||
{:|>, meta1, [negate_condition(condition), {:if, meta2, [block]}]}, | ||
context, | ||
state | ||
) | ||
end | ||
|
||
# .. | ||
defp quoted_to_algebra({:.., _meta, []}, context, state) do | ||
if context in [:no_parens_arg, :no_parens_one_arg] do | ||
|
@@ -2449,4 +2472,43 @@ defmodule Code.Formatter do | |
defp has_double_quote?(chunk) do | ||
is_binary(chunk) and chunk =~ @double_quote | ||
end | ||
|
||
# Migration rewrites | ||
|
||
@bool_operators [ | ||
:>, | ||
:>=, | ||
:<, | ||
:<=, | ||
:in | ||
] | ||
@guards [ | ||
:is_atom, | ||
:is_boolean, | ||
:is_nil, | ||
:is_number, | ||
:is_integer, | ||
:is_float, | ||
:is_binary, | ||
:is_list, | ||
:is_map, | ||
:is_struct, | ||
:is_function, | ||
:is_reference, | ||
:is_pid | ||
] | ||
|
||
defp negate_condition(condition) do | ||
case condition do | ||
{neg, _, [condition]} when neg in [:!, :not] -> condition | ||
{:|>, _, _} -> {:|>, [], [condition, {{:., [], [Kernel, :!]}, [closing: []], []}]} | ||
{op, _, [_, _]} when op in @bool_operators -> {:not, [], [condition]} | ||
{guard, _, [_ | _]} when guard in @guards -> {:not, [], [condition]} | ||
Comment on lines
+2505
to
+2506
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a thought i had while implementing this for styler -- being smart and using
better to just always use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea behind migrate option is that they rewrite the AST and cannot guarantee to work in edge cases where metaprogramming assumes a given AST or Kernel overwrites are done (after all people can have re-defined This particular one feels like an unlikely case, the need to overwriting |
||
{:==, meta, [left, right]} -> {:!=, meta, [left, right]} | ||
{:===, meta, [left, right]} -> {:!==, meta, [left, right]} | ||
{:!=, meta, [left, right]} -> {:==, meta, [left, right]} | ||
{:!==, meta, [left, right]} -> {:===, meta, [left, right]} | ||
_ -> {:!, [], [condition]} | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and
andor
could be added here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we could, but unfortunately this wouldn't be safe and might break code that is working today, since
false or 42
is valid (also arguably pretty bad) butnot (false or 42)
would raise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh right, i always think that those are
bool, bool -> bool
, forgot it'sbool, any -> any
i'll just join you in wishing we could then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could try harder in these cases, checking if the right handside itself is a guard/comparison... but I'm not sure it's worth the extra complexity and it won't capture cases like
and enabled?
anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively, only use
not
within
, and for the 4 comparisons rewrite them to their opposite like with the equality operators:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hesitated between the two approaches, not sure which would be better. Maybe yours is more natural indeed.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually
not
might be better in this case, if we consider that the user explicitly wrote:over
which they could have done in the first place, then
might read better to them for some reason?
It is not critical though, people might see the diff and fix it to their preference eventually.
So I'm fine with either approach.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i think you're right.
if not
is a simpler change to reason over thanif $swapped_equality_operator
. thanks for spelling that scenario out.