exercism / elixir-analyzer

GNU Affero General Public License v3.0
30 stars 32 forks source link

Incorrect or misleading 'Essential' warning in 'Basketball Website' #307

Closed arconautishche closed 2 years ago

arconautishche commented 2 years ago

I'm getting the following warning if I prefix get_in(...) with Kernel:

Make sure to only use use get_in/2 in BasketballWebsite.get_in_path/2 and not in BasketballWebsite.extract_from_path/2. The purpose of this exercise is to practice using the Access Behaviour in two different ways, first directly using the data[key] syntax, and then indirectly using the get_in_path/2.

I'm not sure if this is a mistake or just a matter of wording. Maybe the idea was to say you do not need to prefix Kernel functions with Kernel?

This code produces the warning:

  def extract_from_path(data, path) do
    data
    |> recursive_fetch(path |> tokenize())
  end

  def get_in_path(data, path) do
    Kernel.get_in(data, tokenize(path))
  end

  defp recursive_fetch(map, path)
  # ...
  defp tokenize(path)
  # ...

and this does not:

  def extract_from_path(data, path) do
    data
    |> recursive_fetch(path |> tokenize())
  end

  def get_in_path(data, path) do
    get_in(data, tokenize(path))
  end

  defp recursive_fetch(map, path)
  # ...
  defp tokenize(path)
  # ...
angelikatyborska commented 2 years ago

@arconautishche Good find, there's a bug 👀. This comment is produced when the analyzer thinks you didn't use Kernel.get_in in BasketballWebsite.get_in_path. Unfortunately it only detects get_in used without the module name prefix. We definitely don't want to tell people to remove the module name prefix from the function call, both ways of calling the function are valid.

Our custom macro assert_call is already able to handle the fact that Kernel gets autoimported. We just used it a bit wrong in the analysis for BasketballWebsite - we didn't specify that get_in comes from the Kernel module so it's treated as any other local function. The fix for this problem would be as easy as going to the file lib/elixir_analyzer/test_suite/basketball_website.ex and changing two identical lines of code like this:

# before
called_fn name: :get_in
# after
called_fn module: Kernel, name: :get_in

Adding an extra test to make sure that both calls with and without Kernel are valid would also be great.

@arconautishche Would you want to open a PR with those changes?

arconautishche commented 2 years ago

Thanks for the detailed explanation, @angelikatyborska! I will try to make a PR soon.