exercism / elixir-analyzer

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

Elixir track snake_case misleading automatic recomendation #334

Closed PQ4NOEH closed 1 year ago

PQ4NOEH commented 1 year ago

When the elixir track on the exercise log level when submitted I have got what I think is a wrong automatic recommendation about snake_case. The details of the recommendations are:

image

jiegillet commented 1 year ago

Thank you for the feedback! I see what's going on. Do we agree that log_Label is indeed not snake case because of the L? However, what the analyzer is proposing is not snake case either, it should be log_label.

Would you be interested in helping fixing it? I can guide you and I would accept the contribution valid for hacktoberfest.

PQ4NOEH commented 1 year ago

Yes, I agree log_Label is not correct either. I am happy on helping to fix it, please tell me what should I do.

jiegillet commented 1 year ago

Nice!

The culprit is here: https://github.com/exercism/elixir-analyzer/blob/9b380f6d115b67318d0930bf40672243921c93b6/lib/elixir_analyzer/exercise_test/common_checks/variable_names.ex#L71-L80

We might be able to get away with a |> String.replace("__", "_") to fix this particular problem, but maybe you can find something more robust? Like the comments say, we only care about identifiers that already compile.

We will need to add tests (starting with "log_Label") around here https://github.com/exercism/elixir-analyzer/blob/9b380f6d115b67318d0930bf40672243921c93b6/test/elixir_analyzer/exercise_test/common_checks/function_names_test.exs#L51-L71

You can create a helper function to check the result like get_result in the following test, or even make get_result available to the whole module to make things more concise.

Does that make sense?

PQ4NOEH commented 1 year ago

Yes, it does make sense. I do this one

jiegillet commented 1 year ago

@PQ4NOEH are you still interested in taking this one?

dmarcoux commented 1 year ago

I'm having a look into this :slightly_smiling_face: