exercism / elixir-analyzer

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

Take-A-Number Deluxe analyzer claims about missing @impl GenServer #383

Closed silvadanilo closed 1 year ago

silvadanilo commented 1 year ago

Analyzer claims about missing @impl GenServer but @impl true should be also ok!

dabaer commented 1 year ago

I tripped over this when I first went through this exercise as well.

Having used Elixir professionally for years, the general opinion is to use @impl true for brevity as it is rare for a module to implement more than one behaviour. I was actually surprised to find out the long-hand syntax was to use the defining module's name was a thing for @impl, as I had never seen it used in any Elixir project before.

I'd say it could be useful for beginners to learn to use it in this way, but even Elixir itself uses @impl true rather than @impl Behaviour in it's source code, so the analyzer should allow for both.

If learners are using the Elixir documentation to understand these concepts, they may lean towards @impl true as using the module name is only mentioned briefly in the attribute definitions for Module.

Going further down the rabbit-hole, it doesn't seem like you can even easily use multiple behaviours in one module if the callback names collide.

angelikatyborska commented 1 year ago

We could relax the check to also allow @impl true. If anyone would want to help out, you would need to add a second case here: https://github.com/exercism/elixir-analyzer/blob/4fc78740f47c85f52eb0c9b5b816246b0c1a7e63/lib/elixir_analyzer/test_suite/take_a_number_deluxe.ex#L42-L43 and modify the test https://github.com/exercism/elixir-analyzer/blob/4fc78740f47c85f52eb0c9b5b816246b0c1a7e63/test/elixir_analyzer/test_suite/take_a_number_deluxe_test.exs#L150-L172

dabaer commented 1 year ago

I took a small stab at it, though I think the tests could use some polish.