Fix Code.Fragment.surround_context for keyword keys #13843
Conversation
sabiwara
force-pushed
the
fragment-keyword
branch
from
ed9c88e to
88c7fc6
Compare
lib/elixir/lib/code/fragment.ex
Outdated
| @@ -656,6 +665,9 @@ defmodule Code.Fragment do | |||
| {{:dot, _, [_ | _]} = dot, offset} -> | |||
| build_surround(dot, reversed, line, offset) | |||
|
|
|||
| {{:local_or_var, acc}, offset} when keyword? -> | |||
| build_surround({:keyword, acc}, reversed, line, offset) | |||
There was a problem hiding this comment.
I don't think keyword should be returned here. It's already used for reserved words do end after else catch rescue fn true false nil
There was a problem hiding this comment.
Got it, I misunderstood what keyword meant in this context.
keyword_atom / keyword_key?
There was a problem hiding this comment.
Macro.inspect_atom uses key. Maybe that's fine? Or we use atom_key or keyword_key.
There was a problem hiding this comment.
Either sounds good, I went with :key for the consistency.
Last question: in the case of do:, should it be a :key or a :keyword?
It would be :key with the current PR implementation, but I'm not sure which would make sense from a LSP perspective @lukaszsamson
There was a problem hiding this comment.
It's OK, let's have do: as key and do as keyword.
There was a problem hiding this comment.
Great, thanks gents will merge as is then 🚀
There was a problem hiding this comment.
I just realized: should we call out this "edge case" in the docs?
There was a problem hiding this comment.
I don't think it is an edge case, honestly. It is how the grammar behaves everywhere. :) So there is no need imo!
Related to part 1. of #13818