Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support having code extracts in details #158

Open
jfmengels opened this issue May 7, 2023 · 2 comments
Open

Support having code extracts in details #158

jfmengels opened this issue May 7, 2023 · 2 comments
Labels
design-discussion enhancement New feature or request

Comments

@jfmengels
Copy link
Owner

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.

@jfmengels jfmengels added enhancement New feature or request design-discussion labels May 7, 2023
@gampleman
Copy link

Off of the rules we use:

  • NoDeprecated: maybe? Showing why elm-review thinks it's deprecated might be useful, but sort of doubtful...
  • NoMissingTypeExpose: showing at least one type signature that requires the type to be exposed would be helpful, as I'm frequently surprised by this rule and I don't find it easy to reason about it.

@jfmengels
Copy link
Owner Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants