rebased fix of Issue316 #653
Conversation
Adds an option `for_in_replacement` which allows "in" to be replaced with another operator such as "∈" `\in` or "=" when `always_for_in` is enabled.
|
@domluna I still have 4 tests failing for some reason I can't comprehend, the problem is with code like One example is this: I can't really spot anything that would have broken these tests; hints very welcome. EDIT: disregard this, my rebase broke that. Tests should be fixed now. |
src/fst.jl
Outdated
| if fst.typ === Binary | ||
| idx = findfirst(n -> n.typ === OPERATOR, fst.nodes) | ||
| idx === nothing && return | ||
| op = fst[idx] | ||
|
|
||
| if always_for_in | ||
| op.val = "in" | ||
| if always_for_in && valid_for_in_op(op.val) |
There was a problem hiding this comment.
the validation should occur when the options are initially set. And if the op is invalid it should display a message and not format.
There was a problem hiding this comment.
Hm, this is taken from:
https://github.com/domluna/JuliaFormatter.jl/pull/380/files#diff-fdbcedac9cc0bafcd2fa355f83f9558522f56e8e432e9a3775f59612dce9fe8aR1016
I assumed that it validates the "incoming" operator (that we're not rewriting something that's not a cycle), but actually that doesn't make sense because that wouldn't ever get parsed. This condition doesn't make sense here then, will change shortly.
There was a problem hiding this comment.
OK this is gonna be harder, the validation was there because of this:
always convert `=` to `in` (for loops): Test Failed at /home/exa/work/JuliaFormatter.jl/test/options.jl:253
Expression: fmt(str_, always_for_in = true) == str
Evaluated: "[i for i in 1:10 if i in 2]" == "[i for i in 1:10 if i == 2]"
(i.e., it force-replaces the == operator even in the if part)
Any best way to dodge this? (I pushed the option-validationg-but-failing version now)
There was a problem hiding this comment.
you shouldn't need this check the valid_for_in_op(op.val) part of if always_for_in && valid_for_in_op(op.val) anymore since it's being checked when the options are created.
There was a problem hiding this comment.
the valid_for_in_op check in the condition is gone (see the new commits); the problem is that it now mistakenly fires on the if part as described above.
There was a problem hiding this comment.
@domluna is there any good way to dodge this? Should I track that the binary expression is explicitly following a for keyword? (IIRC I saw something like that elsewhere in the code)
src/options.jl
Outdated
| function validate_options(opts::Options) | ||
| @assert valid_for_in_op(opts.for_in_replacement) | ||
| opts | ||
| end |
There was a problem hiding this comment.
@domluna btw I didn't find a good "central" place to validate the options (except perhaps for the place where styles are expanded, but that's very raw), so hooking this into the State constructor seemed like the best way. Please lmk in case there's a better place.
| elseif fst.typ === Block || fst.typ === Brackets | ||
| for n in fst.nodes | ||
| eq_to_in_normalization!(n, always_for_in) | ||
| elseif fst.typ === Block || fst.typ === Brackets || fst.typ === Filter |
There was a problem hiding this comment.
made this just Filter of !is_leaf since it's a more minimal change
There was a problem hiding this comment.
ah great, yes, that should work pretty well
|
this should get the tests to pass now, I'll see if the initial validation check can be placed elsewhere but that might be ok how it is. |
|
ok I moved it to be a warning I'm happy with this now. |
|
@domluna thanks a lot! |
this should close #316 and is a newer version of #380.
I hope the rebase went well, still catching some minor other issues. Let me know in case I should clean up the history.