Closed ceddlyburge closed 11 months ago
I want to explain a little bit why I introduced this name field.
I had a lot of experience with the Elixir analyzer, which is more complex than the Elm one.
In the Elixir analyzer, each rule has a name, but it is sometimes misleading, for example a rule will have the name "uses max
in maxPower
" but it's not always clear the students should do that or not, the rule detects if max
is in maxPower
, but sometimes find
is None
and sometimes All
(which reverse the logic).
We have that too actually, for example, the rule maxPowerUsesMax
sends a comment if maxPower
doesn't use max
. And the path
field in the comment "elm.blorkemon-cards.use_max"
is not very descriptive because it's a file name.
So the reason for having name
in the Comment
is I wanted a description at the final level "you didn't use max
that's why you get a comment".
At the end of the day, it's also just a convention that could be misunderstood.
Maybe we can remove it if we agree (and document and enforce) to have strong conventions for rules names, for example the name of the rule is the reason you are getting a comment, so we should rename maxPowerUsesMax
to maxPowerDoesNotUseMax
, but as I write this, it sounds wrong to me.
Having stuff for humans makes sense, most code is formatted to be easy for humans to read, as computers don't care either way.
I think the ideal situation is that something is useful for the human and for the computer.
What we have here is only useful for the human, and I think the danger is that it gets out of date with what the computer sees.
I think I would prefer something like "elm.blorkemon-cards.maxPowerDoesNotUseMax"
, can you let me know what your reasons are for it sounding wrong?
I think I would prefer something like
"elm.blorkemon-cards.maxPowerDoesNotUseMax"
, can you let me know what your reasons are for it sounding wrong?
Because the inside of maxPowerDoesNotUseMax.md
is something like "please use max
in maxPower
", and I prefer when the name file is a short version of what's inside, not of the reason why the comment has been picked. The might also be more than one reason why we give out a comment. Imagine elm.blorkemon-cards.SortByShinyPowerDoesntUsecompareShinyPowerOrSortByPowerDoesntUsePower
for a generic comment "please reuse defined functions", and then later we realize there is a third case were that comment would be useful...
Also it could get long and ugly elm.blorkemon-cards.didNotUseListSortByOrListSortWithAnywhereInTheModule
.
I would argue that that path is definitely something useful for the computer, not something useful for humans.
Interesting.
So I think that the function implementation does the best job of communicating what it does, and I think I would prefer to use this in place of a textual description (that might not match the function implementation).
I agree that things like elm.blorkemon-cards.didNotUseListSortByOrListSortWithAnywhereInTheModule
could get ridiculous quickly.
Maybe we could use the rule function name here? so elm.blorkemon-cards.maxPowerUsesMax
Assuming that the function is well named, which it should be, then the file would also be well named. We should probably create a convention for the name, so that we are consistent with the doesUseMax / doesntUseMax confusion. Maybe something like "maxPowerMustUseMax"? And "moduleCannotUseListSort"
And it would make the link between the two more obvious. We could even go all the way and create an elm-review rule to enforce it.
The name in Comment isn't used, so we should probably remove it, as discussed here https://github.com/exercism/elm-analyzer/pull/44#discussion_r1006526840
name property defined here: https://github.com/exercism/elm-analyzer/blob/dfe46ca127fe8fe15da508ec6216036e42e4ee62/src/Comment.elm#L33