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

Linter should be configurable or not exist. #812

Closed
anagrius opened this issue Oct 30, 2022 · 7 comments
Closed

Linter should be configurable or not exist. #812

anagrius opened this issue Oct 30, 2022 · 7 comments

Comments

@anagrius
Copy link
Contributor

The linting errors are all or nothing currently.
This leads to instances where the linter rule author might not have considered all cases,
and false positives occur. False positives means the user has to disable the linting completely. :(

Take for instance the rule that does not allow case Nothing -> Nothing:

case String.toFloat str of
    Just _ -> Just (NumberToken str)
    Nothing -> Nothing

this is arguably more readable than the alternative (which is not considered a linting error):

String.toFloat str |> Maybe.with (always (NumberToken str))

and while I have not benchmarked it, I could imagine it is even more performant.

The main point is that, linting must be 100% correct in all cases or is counter-productive.
This was one of the main points of a recent talk by @jfmengels at GOTO copenhagen on the topic of linters as well.

Another example is that Debug.log is not allowed in production code, but it is allowed in Test code.
Since the linter cannot be configured to ignore certain paths, these show up as errors as well. You may argue that
you should not have Debug.log or Debug.todo in your tests either, but I am not sure you can state that generally.

Generated code is also not treated differently, and is often outside the control of the user. Linting errors there will
also force the user to disable the linter.

I propose 2 options:

  1. Make the linter configurable with at least two options: Which files to ignore, and which rules to ignore / disable.

  2. Don't to linting in LSP directly, but rely on elm-review. It already allows people to configure all these things. It would be a major benefit to trigger a fast subsets of Elm review rules on-save for instance.

I would be happy to contibute if this makes sense.

@razzeee
Copy link
Member

razzeee commented Oct 30, 2022

The rules are configurable via elm-analyse.json and you can opt to use elm-review instead, but it's slow.

@anagrius
Copy link
Contributor Author

Thanks. Maybe the issue is that that is undocumented? Or did I miss it?

@razzeee
Copy link
Member

razzeee commented Nov 1, 2022

Documentation lives here https://stil4m.github.io/elm-analyse/#/configuration

But it's not mentioned on our side, mostly as we want people to let us know when a (default) config does not work.

@anagrius
Copy link
Contributor Author

anagrius commented Nov 1, 2022

I mean the fact that you can use elm-analyse.json is undocumented in Elm-LS's documentation.
I don't think you can have a default that will work for everyone. The linting false positives made it pretty much unusable for us.

@anagrius
Copy link
Contributor Author

anagrius commented Nov 1, 2022

I made a PR documenting the configuration of Elm-Analyse #816

@razzeee
Copy link
Member

razzeee commented Nov 1, 2022

Have you created issues for the false positives?

@anagrius
Copy link
Contributor Author

anagrius commented Nov 4, 2022

Well, it is not false positives as much as it is the linting rule is questionable and should be disabled by default or not exist.

@anagrius anagrius closed this as completed Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants