Open harrysarson opened 3 years ago
I recommend starting with enabling the rules from elm-review-unused
.
You can start with
elm-review
CLI to the probject: npm install -D elm-review
elm-review init --template jfmengels/elm-review-unused/example
in the elm
root of the project.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!
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).
@harrysarson I'd propose a staged approach for this (essentially what @jfmengels suggested, a little bit more formalized)
What's your take on this?
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".
Thanks to @frankschmitt points 1. and 2. from this list (https://github.com/rtfeldman/node-test-runner/issues/446#issuecomment-711215497) are now done 🎉
A PR for step 3 would be amazing 👏
I'll start working on step 3 - should hopefully be pretty straightforward.
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?
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?
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 :man_shrugging:
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.
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.
For now I think ignoring the rule is probably best.
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.
I've decided to go with the "ignore specific files" approach. This way, new source files will still automatically be checked for rule violations.
I've decided to go with the "ignore specific files" approach. This way, new source files will still automatically be checked for rule violations.
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).
Here's a preliminary list of packages containing review rules, categorized by how useful they seem to me at first glance:
Very useful
Possibly useful
Probably not useful
Any thoughts on this? If there's no objection, I'd start integrating the packages from the top of the list.
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.