Add elm review for our elm code

Question

Add elm review for our elm code

harrysarson opened this issue 4 years ago · 16 comments

I cannot write instructions for this as I have never used elm-review before! First step is to workout the instructions and define our goals here.

It would be great to ensure the highest quality of our elm code.

Answer 1 · 2020-10-03T17:05:24.000Z

I recommend starting with enabling the rules from elm-review-unused.

You can start with

Adding the elm-review CLI to the probject: npm install -D elm-review
Adding a configuration. A simple way to do this is by running elm-review init --template jfmengels/elm-review-unused/example in the elm root of the project.
Running it in the test suite by running: It's just elm-review (I don't know where you prefer doing this)

Then you'll have to fix issues. elm-review --fix or elm-review --fix-all can help you out. --fix-all can be very slow if there are are a lot of issues, so it might not be the best start.

If there are too many issues, the PR diff will be very big, so it might be a pain for @harrysarson to review. I would recommend to take the configuration and comment out every rule except NoUnused.Variables and make the PR.

It DOES look like there are not too many of these issues, so running --fix-all is just fine. That said, a lot of the errors are about unused functions in Console.Text, maybe you want to ignore some files for some rules.

Know that you can ignore errors using ignoreErrorsForFiles and ignoreErrorsForDirectories.

Once that is set up, you can take a look at adding new rules. I recommend searching through Elm packages by searching for elm-review and finding packages (or just looking at the ones I made). Hopefully with Hacktoberfest we'll be able to add a lot of documentation-related rules.

I hope this helps!

Answer 2 · 2020-10-15T14:42:57.000Z

I'd be willing to give this one a go (I've never used elm-review, but this seems like the perfect opportunity to get acquainted with it).

Answer 3 · 2020-10-18T15:18:32.000Z

@harrysarson I'd propose a staged approach for this (essentially what @jfmengels suggested, a little bit more formalized)

Establish elm-review support
- provide basic configuration for all Elm subdirectories (using only the NoUnused.Variables rule)
Automate & integrate into CI
- figure out how to integrate elm-review into
  - npm test
  - Travis
  - Appveyor
Support basic ruleset

uncomment every rule fro the ruleset that comes out-of-the-box
fix resulting errors

Decide on additional rulesets

review Elm packages that provide additional rulesets
decide on a set of packages
integrate them

What's your take on this?

Answer 4 · 2020-10-18T15:32:09.000Z

This seems like a good plan to me! Only comment other than to say go for it is:

figure out how to integrate elm-review into

npm test

Travis

Appveyor

We would not need to integrate elm-review into appveyor (if elm-review passes on linux it will on windows too). I would add elm-review to the npm check script and it should "just work".

Answer 5 · 2020-10-20T19:29:07.000Z

Thanks to @frankschmitt points 1. and 2. from this list (#446 (comment)) are now done 🎉

A PR for step 3 would be amazing 👏

Answer 6 · 2020-10-20T20:33:51.000Z

I'll start working on step 3 - should hopefully be pretty straightforward.

Answer 7 · 2020-10-22T18:40:08.000Z

I'm halfway through with enabling the additional Unused rules, and I've got a couple of questions about things that elm-review considers unused. Here's the first one:

───────────────────────────────────── src/Test/Runner/Node/Vendor/Diff.elm:79:35

NoUnused.CustomTypeConstructorArgs: Argument is never extracted and therefore
never used.

78| | CannotGetB Int
79| | UnexpectedPath ( Int, Int ) (List ( Int, Int ))
^^^^^^^^^^^^^^^^^^^

It seems indeed that the arguments to UnexpectedPath are never used. However, my guess is that this was meant to reported somewhere, but instead it is discarded here:

 diff : List a -> List a -> List (Change a)
 diff a b =
     case testDiff a b of
         Ok changes ->
             changes
 
         Err _ ->
             []

Can we safely remove the arguments from the type constructor? And if yes, can / should we get rid of the UnexpectedPath case altogether?

Answer 8 · 2020-10-22T19:11:37.000Z

Also, elm-review complains about a couple of unused type constructors:

────────────────────────────────────────────────────── src/Console/Text.elm:35:7

NoUnused.CustomTypeConstructors: Type constructor Monochrome is not used.

34| = UseColor
35| | Monochrome
^^^^^^^^^^

This type constructor is never used. It might be handled everywhere it might
appear, but there is no location where this value actually gets created.

For testing purposes, I renamed Monochrome to Monochromex and ran the test suite. I got this error:

STDERR
-- NAMING ERROR -------------------------- src/Test/Generated/Main1235162957.elm

I cannot find a Monochrome variant:

14| |> Test.Runner.Node.run { runs = Nothing, report = (ConsoleReport Monochrome), seed = 90830235816756, processes = 16, globs = ["tests/Failing/SplitSocketMessage.elm"], paths = ["/home/frank/src/3rd_party/node-test-runner/tests/fixtures/tests/Failing/SplitSocketMessage.elm"]}

Ultimately, these definitions are (transitively) used by Test.Runner.Node, and I'm kind of wondering why elm-review reports them as unused.

@jfmengels May this be because this module is defined as port module Test.Runner.Node and elm-review doesn't pick up that a port module may be used externally? Or am I using it wrong?

Answer 9 · 2020-10-22T20:13:17.000Z

It seems indeed that the arguments to UnexpectedPath are never used. However, my guess is that this was meant to reported somewhere, but instead it is discarded

That sounds likely.

Can we safely remove the arguments from the type constructor? And if yes, can / should we get rid of the UnexpectedPath case altogether?

If you want to keep feature parity with the current version, you can safely remove it. But this might also be a sign that an error should appear.

For the UnexpectedPath variant, no. The variant even if it doesn't hold data still indicates that we are in a different "state" than the CannotGetB variant for instance. Unless the code for the variant does the same thing 🤷‍♂️

Ultimately, these definitions are (transitively) used by Test.Runner.Node, and I'm kind of wondering why elm-review reports them as unused.

elm-review only accounts for the code that is present. It looks like some code is generated (I'm guessing from the src/Test/Generated/Main1235162957.elm path) which injects a hardcoded reference to Monochrome.

May this be because this module is defined as port module Test.Runner.Node and elm-review doesn't pick up that a port module may be used externally? Or am I using it wrong?

The elm.json indicates that the project is an application and it therefore assumes that none of the code will be used externally, which is true when the codebase it analyzes it all the code that is part of your project, but stops being true when you generate part of the code to be run.

In this case, I would simply ignore that rule for the Console.Text module (using ignoreErrorsForFiles) with a nice comment explaining why. Or you can write a test that uses each variant.

Answer 10 · 2020-10-23T09:10:20.000Z

Both these lint rules come from package code we have inlined due to complications arising from installing extra packages. Maybe now that we use elm-json and the whole dependency resolving thing is much more solid now we could revisit these deps and see if we can un-inline them.

Answer 11 · 2020-10-23T09:12:45.000Z

For now I think ignoring the rule is probably best.

Answer 12 · 2020-10-23T10:06:48.000Z

I have a very similar situation in elm-review where I also generate parts of the code to be run and where I have vendored some dependencies to avoid complications. I have prefixed/moved those to a Vendor folder and ignored all of my rules for that folder.

Answer 13 · 2020-10-27T22:01:24.000Z

I've decided to go with the "ignore specific files" approach. This way, new source files will still automatically be checked for rule violations.

Answer 14 · 2020-10-31T08:12:36.000Z

I've decided to go with the "ignore specific files" approach. This way, new source files will still automatically be checked for rule violations.

Answer 15 · 2020-10-31T08:13:51.000Z

PR for step 3 has been submitted. I'll start working on step 4 (compiling a list of additional rules we might want to add) soon (hopefully this weekend).

Answer 16 · 2020-11-01T07:51:49.000Z

Here's a preliminary list of packages containing review rules, categorized by how useful they seem to me at first glance:

Very useful

ContaSystemer/elm-review-no-missing-documentation: checks for missing documentation for top-level declarations
jfmengels/elm-review-common: focuses on exports, imports and type annotations
jfmengels/elm-review-debug: checks for Debug statements
jfmengels/elm-review-documentation: currently, checks only the README for outdated links; still useful
jfmengels/elm-review-unused: we already use this :-)
sparksp/review-camelcase: ensures all identifiers use camelCase or PascalCase
sparksp/elm-review-ports: checks for unused / problematic ports. This sounds useful (if it returns a lot of false positives, we could get rid of it again)
truqu/elm-review-noleftpizza: checks for the left pizza (left pipeline) operator

Possibly useful

lxierita/no-typealias-constructor-call: checks for usage of the type alias as a constructor. Might be overkill to add a dependency for a single (very specific) rule
truqu/elm-review-noredundantcons: checks for redundant cons ( :: ). Might be overkill to add a dependency for a single (very specific) rule
truqu/elm-review-noredundantconcat: checks for redundant concats ( ++ ). Might be overkill to add a dependency for a single (very specific) rule
folq/review-rgb-ranges: checks for colour values that are out of range. It mentions elm-ui, so I'm not sure whether it's useful for a command-line application
r-k-b/no-long-import-lines: checks for long import lines. Might be overkill to add a dependency for a single rule
sparksp/elm-review-always: checks for usage of always. Might be overkill to add a dependency for a single (very specific) rule
sparksp/elm-review-forbidden-words: checks for configurable list of words (e.g. TODO). We'd have to agree on a list of forbidden words, though

Probably not useful

ContaSystemer/elm-review-no-regex: checks for Regex usages (personally, I like Regexes, so I'm biased)
ContaSystemer/review-no-missing-documentation: seems to be the same as elm-review-no-missing-documentation ?
ContaSystemer/review-noregex: seems to be the same as elm-review-noregex?
carwow/elm-review-rules: looks like a subset of elm-review-nounused
jfmengels/elm-review-the-elm-architecture: checks for correct usage of the Elm Architecture. Presumably not applicable to a command-line application
leojpod/review-no-empty-html-text: checks for empty text tags. Presumably not applicable to a command-line application
r-k-b/no-float-ids: checks for usage of Floats as IDs. The package describes itself as filling a small niche for legacy code
truqu/elm-review-nobooleancase: checks for case with boolean expressions, and suggests using if .. then instead. I disagree with the sentiment that case shouldn't be used for simple boolean expressions, but that's just my personal opinion.

Any thoughts on this? If there's no objection, I'd start integrating the packages from the top of the list.