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

Disable messages based on linters already installed locally #3517

Closed
Pierre-Sassoulas opened this issue Apr 26, 2020 · 4 comments
Closed

Disable messages based on linters already installed locally #3517

Pierre-Sassoulas opened this issue Apr 26, 2020 · 4 comments

Comments

@Pierre-Sassoulas
Copy link
Member

As seen in #3512 we want to disable message that aren't useful to the user. We can imagine that when another linter is installed with the same set of feature the user is using it. The following linter comes to mind:

  • black for formatting
  • isort for import order
  • flake8 for a range of messages
  • flake8 plugin for other messages

Describe the solution you'd like

When we see that a linter is installed (by trying to import it API) we do not execute some checks. This speed pylint up and make the warning shown more relevant. Most of the work in order to know what check exactly to disable was done by @AWhetter in #3512 (for example bare-except is W0702 in pylint, and E722 in flake8 so we can disable W0702 if flake8 is installed)

We could add a warning to the user that some checks are automatically avoided and an option to disable the behavior. This feature would go well with default configurations.

@PCManticore
Copy link
Contributor

Hey @Pierre-Sassoulas Thank you for opening the issue.

This is something that I don't agree in the suggested form. For personal projects I use both flake8, pylint and other tools such as mypy, since all of them catch things that the other tools do not catch. It's very possible that there are folks out there that try to use these tools in tandem as well, and this behaviour might be a bit surprising.

If this would be under a disabled-by-default flag, I wouldn't mind having it, but at this point it seems to me that we're trying to make pylint "friendlier" than it should, since other tools don't consider disabling their messages when pylint is detected.
Disabling certain categories by default could also be an alternative, for instance disabling the format category regardless if black is installed or not.

The original issue for making pylint a bit more sane #746 suggested the addition of separate stages in which we could lump together various checks that are related via a given property (e.g. those that are a bit more advanced, but have a high yield of false positives can live under a particular stage that's not required for all users of pylint to consume). Rather than going the inference route of figuring out what other tools are installed and trying to be amenable to their existence, I would prefer we go to the separate stages route instead.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Apr 27, 2020

f this would be under a disabled-by-default flag,

Yeah I think the default needs to be very consensual and not have a single false positive. So a lot of things should be disabled by default.

I think we have this discussion because the configuration/option code need a very huge refactor, it does not seem to even use argparse. What I propose here likely requires a huge amount of rework before being easy to do. Easier option with argparse and configuration inheritance would make it something like :

if Configuration.disable_redondant_with_installed_linter()
    try:
        import black
        Configuration.inherits("formatting-handled-by-black")
    except ImportError:
       pass

Being friendly is admitedly easier if it cost you only 7 line of code :D

@thejcannon
Copy link
Contributor

Something to consider is with tools like pre-commit which run each tool in isolation (helps avoid dependency conflicts) this feature would be moot. The user would still need to configure each tool.

I think having overlapping responsibilities from using multiple linters in tandem is a symptom of the problem that people need multiple linters. Instead of playing nicely with other tools, pylint should identify the gap and fill it.

As for linter-vs-style-formatter, my opinion would be that pylint stops worrying about style now that we have style tools (something that's echod in #746). pylint can focus on finding bugs/issues.

@Pierre-Sassoulas
Copy link
Member Author

Good point, about pre-commit, I'm going to close this as indeed the problem would be best solved with being able to generate specific configuration compatible with tools easily (some of it was documented in #3647 ), and a better documentation around configuration file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants