-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add blame callback for KeyError
#7803
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
Conversation
This starts with an initial implementation of the `blame/2` callback for `KeyError` to add some helpful `did_you_mean` feedback for potentially typo'd keys. Right now it's only implemented for maps and keyword lists, and only for string and atom keys.
lib/elixir/lib/exception.ex
Outdated
| def message(%{message: message}), do: message | ||
|
|
||
| def message(key, term) do | ||
| msg = "key #{inspect(key)} not found" |
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.
Minor nitpick: what do you think about using message instead of msg (like the function above) and distance instead of dist in the functions below?
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, works for me!
lib/elixir/lib/exception.ex
Outdated
| end | ||
|
|
||
| def blame(exception = %{term: term}, stacktrace) when is_list(term) do | ||
| available_keys = Keyword.keys(term) |
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.
Unfortunately I think there is a bug here. Keyword.keys assumes a keyword is given. So if you do Keyword.fetch!([1, foo: :bar], :bar), it will invoke this clause which then fails on Keyword.keys. My suggestion is to unify both clauses above and do something like this:
if (is_map(term) or Keyword.keyword?(term)) and Enum.all?(term, fn {k, _} -> is_atom(k) end) do
...
else
{message, stacktrace}
end
I know we can have mixed keys in maps, but I think we should start with a more conservative approach for now.
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.
Ah - I didn't think it was possible for a non-keyword list to be passed as the term to a KeyError. I'll update.
|
Thank you! ❤️ |
It was introduced in elixir-lang#7803, looks like it was accidentally made public (no docs, no specs) It shipped in Elixir v1.7 so technically this would be a breaking change. I'm happy to deprecate/doc false it instead.
It was introduced accidentally in #7803 (no docs, no specs)
It was introduced accidentally in #7803 (no docs, no specs)
This starts with an initial implementation of the
blame/2callbackfor
KeyErrorto add some helpfuldid_you_meanfeedback forpotentially typo'd keys. Right now it's only implemented for maps and
keyword lists, and only for string and atom keys.
Resolves #7779