search

jfmengels / elm-review

Analyzes Elm projects, to help find mistakes before your users find them.
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
Other
251 stars 13 forks source link

Support having code extracts in details #158

Open jfmengels opened 1 year ago

jfmengels commented 1 year ago

In the Elm compiler, when you have an error like the shadowing error (see below), it shows the squiggly lines on the problematic range, but also shows the related problem as a source-code extract with more squiggly lines.

-- SHADOWING ---------------------------------------------- src/Page/Article.elm

The name `author` is first defined here:

88| author =
    ^^^^^^
But then it is defined AGAIN over here:

266|         author =
             ^^^^^^
Think of a more helpful name for one of them and you should be all set!

Note: Linters advise against shadowing, so Elm makes “best practices” the
default. Read <https://elm-lang.org/0.19.1/shadowing> for more details on this
choice.

(Note: both squiggly lines are colored in red)

I think it would be interesting to make it easier to do these, and (when needed) to position potential squiggly lines at the right position.

The Elm compiler has prior art on this, as well as Rome tools in their good looking error messages: https://docs.rome.tools/lint/rules/noconstassign/#invalid

Using the source code extractor (https://package.elm-lang.org/packages/jfmengels/elm-review/latest/Review-Rule#withSourceCodeExtractor), we could probably already do this, though adding the line numbers and squiggly lines is I imagine too annoying and we don't want to do this for every rule, so it is probably worth having this as a utility.

I think we should first figure out which rules could benefit from this feature. In other words, which errors could already be improved by showing the location of another piece of code. Then we could probably improve that rule, and then try to backport the learnings into the elm-review API, depending on what we learn.

gampleman commented 1 year ago

Off of the rules we use:

jfmengels commented 1 year ago

NoDeprecated: maybe? Showing why elm-review thinks it's deprecated might be useful, but sort of doubtful...

I think we could show the contents of the deprecation message, and/or mention why it was tagged as deprecated. That could be useful. I think that would be enough in practice.

NoMissingTypeExpose: showing at least one type signature that requires the type to be exposed

Oh yes, that would be really nice!