exercism / elixir-analyzer

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

Analyzer is detecting a provided method as a method that should be private #239

Closed jbcden closed 2 years ago

jbcden commented 2 years ago

In the remote-control-car exercise, there are 2 new methods provided (new/0 and new/1). Both are needed to make the test suite pass (though you could replace new/0 by adding a default value to new/1). When you submit the solution, the analyzer suggests making new/0 private, even though it is called in the public interface.

While the analyzer is not necessarily wrong about new/0 not being strictly necessary, I think this lint might be confusing to newer programmers and was wondering if we can find a way to prevent this lint or restructure the provided code to not have the new/0. Please let me know if anything is unclear or if I am misunderstanding the purpose of the analyzer suggestions.

For a reference to the provided code for this problem: https://github.com/exercism/elixir/blob/main/exercises/concept/remote-control-car/lib/remote_control_car.ex#L4-L6.

jiegillet commented 2 years ago

Ah, good catch, thank you for reporting.

I know what this happens. This check works by comparing the functions defined in the solution to the ones in the exemplar implementation. Of course, in there it is done there by using a default argument, which is the idiomatic solution and produces a public new/1 only.

We claim that the functions that can be public are the ones used in the test suite, and new/0 is definitely used. We could define a new/0 in the exemplar but we would make it less idiomatic, so that's no good.

In the hints, we do mention "Consider a default-argument for the function", so I think we probably want to push students in that direction. We can do a custom check for the use of a default argument in new/1 that would disable the "private functions" comment check. We have one like that in two-fer already. @angelikatyborska what do you think?

I could also tweak the common check so that when it sees a function new(name \\ "default") in the exemplar it produces both new/0 and new/1, but I think then we would miss out on opportunities of adding custom checks and pushing students in the right direction, like in this case.

jbcden commented 2 years ago

@jiegillet Thanks for looking into this! Good to know how this works :smiley:

angelikatyborska commented 2 years ago

We can do a custom check for the use of a default argument in new/1 that would disable the "private functions" comment check. We have one like that in two-fer already. @angelikatyborska what do you think?

Yes, that's a good idea!

I could also tweak the common check so that when it sees a function new(name \ "default") in the exemplar it produces both new/0 and new/1,

That is also a good idea.

but I think then we would miss out on opportunities of adding custom checks and pushing students in the right direction, like in this case.

Our only mechanism for "detecting" those opportunities is to have somebody complain about a confusing analyzer check. This requires somebody to have enough confidence to recognize that the check is wrong, not their solution, and to care enough to even write about this. Which means most students won't complain and will just be confused. IMO that is too high of a price to pay for detecting this. We should fix the public/private check too,

jiegillet commented 2 years ago

Sounds good, let's do both things!