exercism / elixir-analyzer

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

HighScore feedback for module attribute is wrong #350

Closed rhino232 closed 1 year ago

rhino232 commented 1 year ago

Essential

Use a module attribute in add_player/3 and reset_score/2 as a constant for the initial score.

does not correctly recognize the solution

defmodule HighScore do @initial_score 0

jiegillet commented 1 year ago

Hi @rhino232, Could you copy the full solution? I believe the cause of the issue is lower than that. Alternatively, you could share a mentoring link.

Screen Shot 2022-11-29 at 8 51 48

We had a similar problem in #341, with a fix waiting to be merged in #345. Maybe it's the same problem.

rhino232 commented 1 year ago

Hi Jie

here is my mentoring link https://exercism.org/mentoring/external_requests/eb9a48fcd2b042cf8e21463bb0065668

regards

Am Di., 29. Nov. 2022 um 00:53 Uhr schrieb Jie @.***>:

Hi @rhino232 https://github.com/rhino232, Could you copy the full solution? I believe the cause of the issue is lower than that. Alternatively, you could share a mentoring link. [image: Screen Shot 2022-11-29 at 8 51 48] https://user-images.githubusercontent.com/16929078/204404947-34fcd5c9-faa2-4393-9991-4717c77de6db.png

We had a similar problem in #341 https://github.com/exercism/elixir-analyzer/issues/341, with a fix waiting to be merged in #345 https://github.com/exercism/elixir-analyzer/pull/345. Maybe it's the same problem.

— Reply to this email directly, view it on GitHub https://github.com/exercism/elixir-analyzer/issues/350#issuecomment-1329896079, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE4JTWP4DKL5IAJAGJXCTLWKVAU7ANCNFSM6AAAAAASN2G6JI . You are receiving this because you were mentioned.Message ID: @.***>

jiegillet commented 1 year ago

Thank you, I looked into it and it was a quite interesting find. The piece that triggers the comment is

  def reset_score(scores, name) do
    scores
    |> Map.replace!(name, @initial_score)
  rescue
      _e in KeyError ->
        scores
        |> Map.put_new(name, @initial_score)
  end

I had never seen a rescue block without a try before. But it seems to work fine, it's just rarely used syntax. The reason why it triggered the comment is that the analyzer only expects exactly one block in a def, and the rescue block is a second one. We should be able to fix this (it might take some time).

In the mean time, let me share with you the expected answer:

  def reset_score(scores, name) do
    Map.put(scores, name, @initial_score)
  end

In Elixir, rescuing errors is used only when strictly necessary, often there are better options that don't raise errors, like in this case Map.put, or functions that return ok tuples {:ok, _} or {:error, _}.

rhino232 commented 1 year ago

Thanks for the feedback!

I will adapt my solution

regards -- andi

Am Do., 1. Dez. 2022 um 14:12 Uhr schrieb Jie @.***>:

Thank you, I looked into it and it was a quite interesting find. The piece that triggers the comment is

def reset_score(scores, name) do scores |> Map.replace!(name, @initial_score) rescue _e in KeyError -> scores |> Map.put_new(name, @initial_score) end

I had never seen a rescue blockwithout atrybefore. But it seems to work fine, it's just rarely used syntax. The reason why it triggered the comment is that the analyzer only expects exactly one block in adef, and the rescue` block is a second one. We should be able to fix this (it might take some time).

In the mean time, let me share with you the expected answer:

def reset_score(scores, name) do Map.put(scores, name, @initial_score) end

In Elixir, rescuing errors is used only when strictly necessary, often there are better options that don't raise errors, like in this case Map.put, or functions that return ok tuples {:ok, } or {:error, }.

— Reply to this email directly, view it on GitHub https://github.com/exercism/elixir-analyzer/issues/350#issuecomment-1333748729, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE4JTSLF7DR5MCJPSRRXPTWLCP3FANCNFSM6AAAAAASN2G6JI . You are receiving this because you were mentioned.Message ID: @.***>

angelikatyborska commented 1 year ago

I believe this issue has been resolved by those two PRs: https://github.com/exercism/elixir-analyzer/pull/366 and https://github.com/exercism/elixir-analyzer/pull/369