Update with fixes to collapsing nested cases with list tails #5169
Conversation
matiascr
changed the title
matiascr
force-pushed
the
fix-collapse_nested_cases_on_list_tails
branch
2 times, most recently
from
b383eef to
5475223
Compare
lpil
left a comment
lpil
left a comment
There was a problem hiding this comment.
Brilliant! Thank you! Few small Qs inline
| } = matched_variable | ||
| { | ||
| // We need to un-nest the list when replacing the tail | ||
| let surrounding_brackets = Regex::new(r"(^\[|\]$)").expect("a valid regex"); |
There was a problem hiding this comment.
Can we do this efficiently without regex? It's quite tricky to read this, and we tend not to use it.
If not then the regex will need to be cached in a static variable (currently it is recompiled every time) and it'll need a detailed comment explaining what this does. Thank you.
| .to_string(); | ||
|
|
||
| // We need to also remove the prefix when replacing the variable | ||
| // at the tail of the list |
There was a problem hiding this comment.
Perhaps I could have been more clear... What I meant is that, rather than replacing the span of the variable, we need to use the span from the tail prefix to the end of the variable (prefix_and_variable_span)
There was a problem hiding this comment.
I'm sorry, I don't understand. Could you add a detailed comment with examples please 🙏
|
When you are ready for a review please un-draft this PR and tag me for a review. Thank you! |
matiascr
force-pushed
the
fix-collapse_nested_cases_on_list_tails
branch
from
5475223 to
764d3d3
Compare
matiascr
force-pushed
the
fix-collapse_nested_cases_on_list_tails
branch
2 times, most recently
from
22bad62 to
bdd982a
Compare
matiascr
force-pushed
the
fix-collapse_nested_cases_on_list_tails
branch
from
bdd982a to
2332f28
Compare
2 participants
Addresses #5168