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

Add different configuration for different files #3767

Open
Conchylicultor opened this issue Aug 6, 2020 · 10 comments
Open

Add different configuration for different files #3767

Conchylicultor opened this issue Aug 6, 2020 · 10 comments
Labels
Configuration Related to configuration Enhancement ✨ Improvement to a component Help wanted 🙏 Outside help would be appreciated, good for new contributors High effort 🏋 Difficult solution or problem to solve

Comments

@Conchylicultor
Copy link

Conchylicultor commented Aug 6, 2020

Is your feature request related to a problem? Please describe

This request is similar request to #618 (Add different configuration for different sub directories) but at the file level.

Currently, pylint apply the same config instinctively to all files. However some rules should be applied only to specific files.

For instance it is really frequent in test files to access private members (assert module._some_fn()). Currently all our tests files trigger hundreds of protected-access warnings.

Describe the solution you'd like

We would like a way to overwrite specific rules for all *_test.py files.

For instace being able to register some pattern="**/*_test.py" with custom rules associated. All files which would match the pattern would use the special pylint rules.

Note: if the pattern accept directory names (some_dir/**), it would also solve #618 while being more flexible (like: **/some_dir/**).

@AWhetter
Copy link
Contributor

AWhetter commented Sep 7, 2020

Although you're describing a slightly different use case, I'm closing this in favour of #618 because once that's implemented it would be the way that we would recommend solving your use case.

@AWhetter AWhetter closed this as completed Sep 7, 2020
@Conchylicultor
Copy link
Author

I don't think #618 solve this issue.

There are many reason where we do not want test to be located in a separate repository for example:

  • Moving tests in a separate folder makes it harder to navigate between the implementation and tests.
  • In tensorflow_dataset, datasets are implemented as self-contained modules containing the implementation, test, metadata,... (example). Moving tests in a separate folder would break the module abstraction.

@BrettMoan
Copy link

BrettMoan commented Feb 25, 2022

@AWhetter is re-opening this possible? I'd like to be able to configure pylint to ignore test files the way @Conchylicultor described.

i wouldn't think pylint should be prescriptive of "whether test files are stored in standalone tests/test, directories to support ignoring them from pylint."

I would hope that instead we would see a configurable flags for such as something like

IGNORED_FOLDER_BLACKLIST = test

IGNORED_FILES_BLACKLIST = *_test.py, test_*.py  

@Pierre-Sassoulas Pierre-Sassoulas added the Configuration Related to configuration label Feb 25, 2022
@Pierre-Sassoulas
Copy link
Member

@BrettMoan this option already exists, see

--ignore=<file[,file...]>

    Files or directories to be skipped. They should be base names, not paths.

or ignore-paths introduced in pylint 2.9 if you want to use regexes.

@Conchylicultor
Copy link
Author

I don't want to ignore the test filles. But apply different pylint rules to them.

I agree that pylint shouldn't impose were tests filles are stored.
For example at Google, tests filles are stored next to the implementation which make them much more discoverable and much more easy to navigate between test & implementation.
Google has an internal pylint version which apply different rules in test filles but it would be nice if this was natively supported by pylint as it would help open sourcing our code.

@Pierre-Sassoulas
Copy link
Member

Google has an internal pylint version which apply different rules in test filles but it would be nice if this was natively supported by pylint as it would help open sourcing our code.

If a googler want to contribute to pylint it would be very welcome. A lot of googler already did !

From where I'm at, the per-directory configuration is something that has been planned for a long time, and it's not implemented yet because we do not have enough contributors willing to take time to implement it. The per-file configuration seems a step above that, so it's unlikely to be done in the short term unless a new contributor does it.

@Pierre-Sassoulas Pierre-Sassoulas changed the title Custom config for test files Add different configuration for different files Feb 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Help wanted 🙏 Outside help would be appreciated, good for new contributors High effort 🏋 Difficult solution or problem to solve labels Feb 25, 2022
@ssbarnea
Copy link
Contributor

ssbarnea commented Mar 11, 2022

I personally do not think we really need different configuration for different files but we need wildcards for disabling rules, like flake8. They call the option per-file-ignores but it works with patterns, allowing you to disable specific rules for specific files and folders.

See https://github.com/ansible-community/ansible-lint/blob/main/.flake8#L49-L62

per-file-ignores =
  src/ansiblelint/cli.py: D101 D102
  src/ansiblelint/rules/*.py: D100 D101 D102

That is kinda essential for enabling progressive improvement of codebase. You might have hundreds of thousands of problems to fix on a project and you likely want to fix them gradually.... while preventing addition of new code that is not compliant. Touching lots of files to add in-place disable does not really scale well.

@Pierre-Sassoulas
Copy link
Member

That is kinda essential for enabling progressive improvement of codebase.

For that we have #5403

@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Jul 6, 2022
@christopherpickering
Copy link
Contributor

@ssbarnea I really wanted per-file-ignores to avoid having to had a # pylint: disable= to every file in my codebase.

I copied a bit of code from the pylint-django plugin, with a bit of new logic, and added a new section to my pyproject.toml.... Now I have per-file-ignores that works the same way as flake8.

Here's how I did it:

Updates to pyproject.toml

[tool.pylint.MASTER]
# the current project needs to be added to path for pylint to pickup the custom plugin.. so add to path here.
init-hook="from pylint.config import find_default_config_files; import sys; [sys.path.append(str(x.parent)) for x in find_default_config_files()]"

# load the custom plugin (project_root/tests/pylint_plugins/per_file_ignores.py). I stuck it in my
# tests dir so that my project isn't poluted w/ junk.
load-plugins=[
    "tests.pylint_plugins.test_plugin"
]
# don't pylint the plugins... optional...
ignore="tests/pylint_plugins"

[tool.pylint-cust.per-file-ignores]
# here is our per file ignores! either the code name or value! Comma separated list.
"/tests/"="missing-function-docstring,W0621"

New Plugin <project root>/tests/pylint_plugins/per_file_ignores.py

Be sure and also add a <project root>/tests/pylint_plugins/__init__.py and <project root>/tests/__init__.py

The meat of the code is the last 5 lines. If you change where the plugin is located, you will need up update the path to your pyproject.toml

import re
from pathlib import Path
from typing import List

from pylint.exceptions import UnknownMessageError
from pylint.lint import PyLinter

try:
    import tomllib
except ImportError:
    import tomli as tomllib  # type: ignore


def get_checker_by_msg(linter, msg):
    for checker in linter.get_checkers():
        for key, value in checker.msgs.items():
            if msg == key or msg == value[1]:
                return checker


class Suppress:
    def __init__(self, linter):
        self._linter = linter
        self._suppress = []
        self._messages_to_append = []

    def __enter__(self):
        self._orig_add_message = self._linter.add_message
        self._linter.add_message = self.add_message
        return self

    def add_message(self, *args, **kwargs):
        self._messages_to_append.append((args, kwargs))

    def suppress(self, *symbols):
        for symbol in symbols:
            self._suppress.append(symbol)

    def __exit__(self, exc_type, exc_val, exc_tb):
        self._linter.add_message = self._orig_add_message
        for to_append_args, to_append_kwargs in self._messages_to_append:
            if to_append_args[0] in self._suppress:
                continue
            self._linter.add_message(*to_append_args, **to_append_kwargs)


class DoSuppress:
    def __init__(self, linter: PyLinter, message_id_or_symbol, test_func):
        self.linter = linter
        self.message_id_or_symbol = message_id_or_symbol
        self.test_func = test_func

    def __call__(self, chain, node):
        with Suppress(self.linter) as s:
            if self.test_func(node):
                s.suppress(*self.symbols)
            chain()

    @property
    def symbols(self) -> List:
        # At some point, pylint started preferring message symbols to message IDs.
        # However, this is not done consistently or uniformly
        #   - occasionally there are some message IDs with no matching symbols.
        # We try to work around this here by suppressing both the ID and the symbol.
        # This also gives us compatability with a broader range of pylint versions.

        # Similarly, between version 1.2 and 1.3 changed where the messages are stored
        # - see:
        #   https://bitbucket.org/logilab/pylint/commits/0b67f42799bed08aebb47babdc9fb0e761efc4ff#chg-reporters/__init__.py
        # Therefore here, we try the new attribute name, and fall back to the old
        # version for compatability with <=1.2 and >=1.3

        try:
            pylint_messages = self.get_message_definitions(self.message_id_or_symbol)
            the_symbols = [
                symbol
                for pylint_message in pylint_messages
                for symbol in (pylint_message.msgid, pylint_message.symbol)
                if symbol is not None
            ]
        except UnknownMessageError:
            # This can happen due to mismatches of pylint versions and plugin
            # expectations of available messages
            the_symbols = [self.message_id_or_symbol]

        return the_symbols

    def get_message_definitions(self, message_id_or_symbol):
        msgs_store = getattr(self.linter, "msgs_store", self.linter)

        if hasattr(msgs_store, "check_message_id"):
            return [msgs_store.check_message_id(message_id_or_symbol)]
        # pylint 2.0 renamed check_message_id to get_message_definition in:
        # https://github.com/PyCQA/pylint/commit/5ccbf9eaa54c0c302c9180bdfb745566c16e416d
        elif hasattr(msgs_store, "get_message_definition"):
            return [msgs_store.get_message_definition(message_id_or_symbol)]
        # pylint 2.3.0 renamed get_message_definition to get_message_definitions in:
        # https://github.com/PyCQA/pylint/commit/da67a9da682e51844fbc674229ff6619eb9c816a
        elif hasattr(msgs_store, "get_message_definitions"):
            return msgs_store.get_message_definitions(message_id_or_symbol)
        else:
            msg = (
                "pylint.utils.MessagesStore does not have a "
                "get_message_definition(s) method"
            )
            raise ValueError(msg)


class AugmentFunc:
    def __init__(self, old_method, augmentation_func):
        self.old_method = old_method
        self.augmentation_func = augmentation_func

    def __call__(self, node):
        self.augmentation_func(Chain(self.old_method, node), node)


class Chain:
    def __init__(self, old_method, node):
        self.old_method = old_method
        self.node = node

    def __call__(self):
        self.old_method(self.node)


def augment_all_visit(linter, message_id_or_symbol, augmentation):
    """
    Augmenting a visit enables additional errors to be raised (although that case is
    better served using a new checker) or to suppress all warnings in certain
    circumstances.
    Augmenting functions should accept a 'chain' function, which runs the checker
    method and possibly any other augmentations, and secondly an Astroid node.
    "chain()" can be called at any point to trigger the continuation of other
    checks, or not at all to prevent any further checking.
    """
    checker = get_checker_by_msg(linter, message_id_or_symbol)

    for method in dir(checker):
        if method.startswith("visit"):
            old_method = getattr(checker, method)
            setattr(checker, method, AugmentFunc(old_method, augmentation))


def disable_message(linter, message_id, test_func):
    """
    This wrapper allows the suppression of a message if the supplied test function
    returns True.
    """
    augment_all_visit(linter, message_id, DoSuppress(linter, message_id, test_func))


class IsFile:
    def __init__(self, file_string, linter):
        self.file_string = file_string
        self.linter = linter

    def __call__(self, node):
        return bool(
            re.search(
                self.file_string, Path(self.linter.current_file).as_posix(), re.VERBOSE
            )
        )


def register(linter: "PyLinter") -> None:
    """This required method auto registers the checker during initialization.

    :param linter: The linter to register the checker to.
    """

    content = tomllib.load((Path(__file__).parents[2] / "pyproject.toml").open("rb"))
    ignores = {**content["tool"]["pylint-cust"]["per-file-ignores"]}

    for file_path, rules in ignores.items():
        for rule in rules.split(","):
            disable_message(linter, rule.strip(), IsFile(file_path, linter))

@christopherpickering
Copy link
Contributor

Got excited and ended up making a python package with this so I can use it in a few other projects..

https://pypi.org/project/pylint-per-file-ignores/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Related to configuration Enhancement ✨ Improvement to a component Help wanted 🙏 Outside help would be appreciated, good for new contributors High effort 🏋 Difficult solution or problem to solve
Projects
None yet
Development

No branches or pull requests

6 participants