More precise error message when escaping a regex with a ref #14560
Conversation
lib/elixir/src/elixir_quote.erl
Outdated
| TT = | ||
| [if | ||
| is_reference(V) -> | ||
| [case extract_value_ref(V) of |
There was a problem hiding this comment.
Instead of extracting value, could we perhaps do a do_quote_map_value that checks for references, tuples, and otherwise just fallbacks to do_quote, returning either {ok, Quoted} or {error, Ref}?
There was a problem hiding this comment.
@josevalim is it what you meant? 30ee6fd
I personally prefer the previous version since it's slightly more DRY and needs less params to be passed around, but no strong opinion.
I have another idea too, will try to push as an alternative proposal and let you chose 🙂
There was a problem hiding this comment.
I kinda like this one 26f3584 readability-wise, even if line-wise it's slightly longer.
But I liked the initial version too.
There was a problem hiding this comment.
@josevalim is it what you meant? 30ee6fd
The idea is that you would already do the quoting as you traverse the tuple. :) This way it would be easier to traverse other data structures (even lists) without additional overhead. Although it does come with some duplication.
It is your call though! Pick whatever you prefer and ship it!
There was a problem hiding this comment.
I see what you mean.
But yeah, it indeed would cause duplication and might not even be such a win perf-wise, even if we do it in one pass:
- current version: just walks the structure without creating structures and do a couple of comparisons, should be quite fast
- suggested version: would need to create OK tuples (and more complexity / duplication)
It is your call though! Pick whatever you prefer and ship it!
Will merge the latest version then!
Should I backport or is there no more 1.18 planned?
2 participants
Close #14497
This approach might be overkill, maybe making it regex aware is better here. I'm not sure.