Add common rules: NoUnused and Simplify

[jiegillet]

This PR introduces (or rather uncomments) common rules: NoUnused and Simplify, that are directly pulled from jfmengels/elm-review-unused and jfmengels/elm-review-simplify.

NoUnused suggests to remove code that is not being used. For example,

module TwoFer exposing (..)

-- unused imported module
import Parser.Advanced

Would trigger the following message to students:

The value indicated below is not used anywhere or is unusable. You could remove it or use it somewhere.

3| -- unused imported module
4| import Parser.Advanced
          ^^^^^^^^^^^^^^^

Simplify suggests basic ways to simplify the code. For example,

module TwoFer exposing (..)

one =
  1 * 1

Would trigger the comment

A way to simplify your code has been found.

Simplify: Unnecessary multiplication by 1

3| one =
4|  1 * 1
     ^^^^

Multiplying by 1 does not change the value of the number.

The analyzer comments are here. The rules for Simplify are numerous and diverse, so I opted to pass the whole message in a parameter (code and explanations), even though it's not ideal for Exercism. I'm only using 5 rules for NoUnused, so I created 5 separate analyzer comments with some explanations that only receive the code (and squiggly lines) as parameter.

I've added a few unit tests for checking the decoders, the actual rules are very well tested, so I'm relying on that.

I chose to set the rules as "Actionable" because the suggestions are a little bit more than "Informative" in my opinion, but I'm happy to discuss that.

[ceddlyburge]

What is this Simplify.defaults? I can't find the code for it anywhere (think I must be having a bad day :))

[jiegillet]

This isn't from this repo, it's from jfmengels/elm-review-simplify. Sorry the modules names are confusing. The defaults can be changed to allow a few very specific exceptions, but I don't think they apply here.

[ceddlyburge]

All the decoder tests are very long, and they obscure the other (probably more intent revealing) tests. I think it probably makes sense to keep the decoder tests long, as I don't think there is a sensible way to shorten them.

However, how would you feel about moving them to another file? This should make the primary tests stand out a bit more.

[jiegillet]

In my mind, it's the opposite, the decoder tests are the primary tests, because they are testing our code.
The rule tests are only here to support the decoder tests (explaining where the json comes from) and as a quick sanity check, not actually checking that the rules work correctly, because there are so many use cases. For checking those, one should at the hundreds of unit tests on the rules repositories.

Should I add a comment explaining this?

[ceddlyburge]

Maybe we can just leave it :)

For me the decoder tests are hard to read, and they look fragile as they have a lot of very specific hard coded information. As somebody reading the tests, the decoder tests don't give me much confidence (for example, if there was an error in the test and it was passing when it shouldn't, I definitely wouldn't be able to tell).

However the rule tests are much easier to read, are much more closely linked to what we want the code to do, and they give me a lot of confidence that the code works as it should. the inputs and expected outputs are concise and understandable, and the connection between them is clear. I think the rule tests also test the decoder as well, so are a kind of integration / end to end type test.

I think the two types of tests serve different purposes. The decoder tests are probably useful when debugging / working on the decoders, and the rules tests are good as documentation.

[jiegillet]

I think the rule tests also test the decoder as well, so are a kind of integration / end to end type test.

I wish that was the case, unfortunately it's not so.
The reason is that the process of getting the final analysis is split in two: 1) get elm-review to apply the rules and 2) get the output and transform it into Exercism format. The decoders are only used in step 2) and unfortunately there is no way to do the 1) -> 2) transition in a pure way, the elm-review output can only be produced by the node application, so we cannot have a fully integrated test.

I agree that the decoder tests are hard to read, but I couldn't find a better way. They aren't much more than a sanity check, thankfully all in all the decoders are actually pretty simple, they just read a couple of fields and combine them.

[ceddlyburge]

Hi @ErikSchierboom. When this PR is merged and released it will affect all the concept and practice exercises for the track. We have tested it in production already on the Strain exercise, where it works nicely, and the code is well tested, but it is still a risky / big impact release.

I'm not really sure what the release process is, but I would like to be available when it happens, so I can give it a quick check, and it would be worth having a fallback plan in place. Could you advise?

[jiegillet]

@ceddlyburge as a sanity check, I ran the rules on every exemplar/example solutions in the Elm track. I'm copying here all the hits (in a wonky format, don't mind that). They all seem sensible, except the very last one. I should open an issue about that one :)

concept_TracksOnTracksOnTracks
elm.common.no_unused.variables
3| import List as List
                  ^^^^"

                  
practice_Acronym
elm.common.simplify
Simplify: Use String.concat instead

11|         |> List.map firstLetter
12|         |> String.join \"\"
               ^^^^^^^^^^^

Using String.join with an empty separator is the same as using String.concat."


practice_Allergies
elm.common.no_unused.parameters
26|         |> List.indexedMap (\\i n -> ( Bitwise.shiftLeftBy i 1, n ))
27|         |> List.filter (\\( s, n ) -> Bitwise.and s score > 0)
                                  ^
28|         |> List.map Tuple.second"


practice_AtbashCipher
elm.common.no_unused.variables
4| import Dict
5| import Regex exposing (Regex)
                          ^^^^^
6| import String"


practice_GradeSchool
elm.common.simplify
Simplify: List.map and List.concat can be combined using List.concatMap

51|         |> List.map Tuple.second
52|         |> List.concat
               ^^^^^^^^^^^

List.concatMap is meant for this exact purpose and will also be faster."


practice_Luhn
elm.common.no_unused.variables
3| import Char exposing (isDigit)
4| import Regex exposing (Regex)
                          ^^^^^
5| import String"


practice_MatchingBrackets
elm.common.no_unused.variables
54| flip : (a -> b -> c) -> b -> a -> c
55| flip f b a =
    ^^^^
56|     f a b"


practice_PhoneNumber
elm.common.no_unused.variables
5| import Regex exposing (Regex)
6| import String exposing (concat, length, slice, startsWith)
                                                  ^^^^^^^^^^"
        }
      },
      {
elm.common.no_unused.variables
5| import Regex exposing (Regex)
6| import String exposing (concat, length, slice, startsWith)
                                           ^^^^^"
        }
      },
      {
elm.common.no_unused.variables
5| import Regex exposing (Regex)
6| import String exposing (concat, length, slice, startsWith)
                                   ^^^^^^"
        }
      },
      {
elm.common.no_unused.variables
3| import List exposing (head, map)
                         ^^^^
4| import Maybe exposing (andThen)"
        }
      },
      {
elm.common.no_unused.variables
3| import List exposing (head, map)
4| import Maybe exposing (andThen)
                          ^^^^^^^
5| import Regex exposing (Regex)"


practice_Raindrops
elm.common.simplify
Simplify: Use String.concat instead

27|         result =
28|             String.join \"\" drops
                ^^^^^^^^^^^
29|     in

Using String.join with an empty separator is the same as using String.concat."


practice_RnaTranscription
elm.common.simplify
Simplify: Use String.concat instead

18|         |> Result.map (List.map String.fromChar)
19|         |> Result.map (String.join \"\")
                           ^^^^^^^^^^^
20|         |> Result.mapError createErrorMessage

Using String.join with an empty separator is the same as using String.concat."


practice_RunLengthEncoding
elm.common.simplify
Simplify: Use String.concat instead

20|         |> List.map stringifyCounts
21|         |> String.join \"\"
               ^^^^^^^^^^^

Using String.join with an empty separator is the same as using String.concat."


practice_Triangle
elm.common.simplify
Simplify: The size of the set is 3

20|     else
21|         case Set.size (Set.fromL) of
                 ^^^^^^^^
22|             1 ->

The size of the set can be determined by looking at the code."

[ceddlyburge]

This is a good idea, shall we open an issue to fix all those?

[ErikSchierboom]

Approving because the gist of it looks good. I'll let the actual review to others

[ErikSchierboom]

I'm not really sure what the release process is, but I would like to be available when it happens, so I can give it a quick check, and it would be worth having a fallback plan in place. Could you advise?

Once this PR is merged, it gets deployed automatically (takes a couple of minutes, see the github actions in this repo at that point).

[mpizenberg]

I also agree with the adding of NoUnused and Simplify rules. I have a question regarding the tests though. I haven't read the tests in details, but I see a lot of text that resemble elm-review output used to check tests outputs. Isn't that very easy to break if Jeroen updates a bit the output messages like colors or ^ markers or anything not really important? How frequent do you think those changes could happen?

I'm asking since the elm track is probably going to stay a low traffic / low maintenance track for quite some time until elm dominates the Frontend world (mouahahaha, ok sorry probably never happening). So It's important that we take seriously into account the maintenance burden for each improvement we do. I was really lucky to take over the track before the v3 transition because there was no maintainer left at that time to help me take over.

[jiegillet]

I haven't read the tests in details, but I see a lot of text that resemble elm-review output used to check tests outputs. Isn't that very easy to break if Jeroen updates a bit the output messages like colors or ^ markers or anything not really important? How frequent do you think those changes could happen?

If the output messages are a bit updated, it should be mostly fine. The only two fields that the decoder reads are the rule name (unlikely to change) and the formatted output. Common.Simplify takes it all as it is, so no problem, and Common.NoUnused takes the lines that start with a number or that end with a ^, those are the only two important things (colors are stripped out).

Those decoders are like 30 lines each and fairly basic, I see it as a measured risk, an alternative is to fork source code, but that's guaranteed to be a much greater burden.

[jiegillet]

I sent a PR over on elm-review-simplify to fix the rule that wasn't correct. A new release version is available, so I'm updating the dependencies here.

Is there more I can do in here?

[ceddlyburge]

I'm happy with it, I think Matthieu still might want to talk through some things! I would like to agree a time when we merge it, so that we can give it a quick check on the production website afterwards.

[mpizenberg]

Looks good to me! I don't know very well the rules because I'm not using elm-review much but the simplify and unused rules should be great common rules.

Regarding tests, I have some reserves but I don't want to block this PR that should bring much value for students. Let's go with it!

I agree with @ceddlyburge, would be nice if at least two persons familiar with the track (my best guess is Cedd and Jeremie) are present at the time of rolling this out.

[mpizenberg]

@jiegillet I guess we could merge this one too? now that the sister PR is merged?

[jiegillet]

Sure. I'll take the leap then :)