Repository: MarketSquare/robotframework-robocop (python)

Issue:
Do not remove rules, only deprecate them
We had already removed some rules in the past (with some sort of deprecation period). In the future it will be better to 'move' rule to deprecation forever instead removing it. Our documentation will be more consistent (the rules will not 'dissapear' suddenly when switching between versions) and there will no sudden blank space in rules ids:

```
    "080X": Rule(
        rule_id="080X",

    ),
    "080X+1": Rule(
        rule_id="080X+1",
        deprecated=True,
    ),
    "080X+2": Rule(
        rule_id="080X+2",

    ),
```

If the rule is marked as deprecate, it will show warning when rule is used in --include/--exclude (same as with current deprecation solution but it will be handled on rule definition level, not on configuration level). The rule implementation could be removed.

Pull request description:
Add mechanism for rule deprecation
Closes #1025 

Documentation visuals for deprecated rule could be improved but it can be done in the future updates - especially that we don't have any deprecated rule right now.

Patch:
diff --git a/docs/conf.py b/docs/conf.py
index 3aa3222..2d40fee 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -87,6 +87,7 @@ def get_checker_docs():
                 "robocop_version": robocop_version,
                 "msg": rule.msg,
                 "docs": rule.docs,
+                "deprecated": rule.deprecated,
                 "params": [
                     {
                         "name": param.name,
diff --git a/docs/releasenotes/unreleased/other.3.rst b/docs/releasenotes/unreleased/other.3.rst
new file mode 100644
index 0000000..b72dc85
--- /dev/null
+++ b/docs/releasenotes/unreleased/other.3.rst
@@ -0,0 +1,19 @@
+Rules can be now deprecated (#1025)
+------------------------------------
+
+Some of our rules were deprecated and then removed in the past. It could lead to issues when jumping from older
+Robocop versions. Because of that we have introduced mechanism to deprecate the rules. It can be also used
+in custom rules:
+
+```
+rules = {
+    "1102": Rule(rule_id="1102", name="custom-rule", msg="Example rule", deprecated=True, severity=RuleSeverity.ERROR),
+}
+```
+
+Deprecated rule implementation can be removed and only rule definition can stay. If the rule is used in ``--include``,
+``--exclude`` or ``--configure`` warning will be printed:
+
+```
+Rule W1102 deprecated is deprecated. Remove it from your configuration.
+```
diff --git a/docs/rules_list.rst b/docs/rules_list.rst
index 4720402..5fbd473 100644
--- a/docs/rules_list.rst
+++ b/docs/rules_list.rst
@@ -34,6 +34,11 @@ Below is the list of all built-in Robocop rules. Enjoy |:sunglasses:|
 {% for rule_doc in checker_group[1] %}
 .. _{{ rule_doc.name }}:
 
+{% if rule_doc.deprecated %}
+.. warning::
+
+      Rule is deprecated.
+{% endif %}
 {{ rule_doc.name }} / {{ rule_doc.severity }}{{ rule_doc.id }}
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/robocop/checkers/__init__.py b/robocop/checkers/__init__.py
index 6bada9b..706f57b 100644
--- a/robocop/checkers/__init__.py
+++ b/robocop/checkers/__init__.py
@@ -32,9 +32,8 @@ import importlib.util
 import inspect
 from collections import defaultdict
 from importlib import import_module
-from itertools import chain
 from pathlib import Path
-from typing import Dict, List, Tuple
+from typing import TYPE_CHECKING, Dict, List, Tuple
 
 try:
     from robot.api.parsing import ModelVisitor
@@ -50,6 +49,10 @@ from robocop.exceptions import (
     RuleReportsNotFoundError,
 )
 
+if TYPE_CHECKING:
+    from robocop.rules import Rule
+    from robocop.run import Robocop
+
 
 class BaseChecker:
     rules = None
@@ -169,6 +172,7 @@ class RobocopImporter:
         self.imported_modules = set()
         self.seen_modules = set()
         self.seen_checkers = defaultdict(list)
+        self.deprecated_rules = dict()
 
     def get_initialized_checkers(self):
         yield from self._get_checkers_from_modules(self.get_internal_modules(), is_community=False)
@@ -285,11 +289,18 @@ class RobocopImporter:
             rules[rule.name] = rule
         return rules
 
+    def register_deprecated_rules(self, module_rules: Dict[str, "Rule"]):
+        for rule_name, rule_def in module_rules.items():
+            if rule_def.deprecated:
+                self.deprecated_rules[rule_name] = rule_def
+                self.deprecated_rules[rule_def.rule_id] = rule_def
+
     def get_checkers_from_module(self, module, is_community: bool) -> List:
         classes = inspect.getmembers(module, inspect.isclass)
         checkers = [checker for checker in classes if is_checker(checker)]
         category_id = getattr(module, "RULE_CATEGORY_ID", None)
         module_rules = self.get_rules_from_module(module)
+        self.register_deprecated_rules(module_rules)
         checker_instances = []
         for checker in checkers:
             checker_instance = checker[1]()
@@ -310,10 +321,11 @@ class RobocopImporter:
         return checker_instances
 
 
-def init(linter):
+def init(linter: "Robocop"):
     robocop_importer = RobocopImporter(linter.config.ext_rules)
     for checker in robocop_importer.get_initialized_checkers():
         linter.register_checker(checker)
+    linter.rules.update(robocop_importer.deprecated_rules)
 
 
 def get_builtin_rules():
diff --git a/robocop/config.py b/robocop/config.py
index 2e4eddb..ffc10af 100644
--- a/robocop/config.py
+++ b/robocop/config.py
@@ -5,23 +5,19 @@ import re
 import sys
 from itertools import chain
 from pathlib import Path
-from typing import Dict, Pattern, Set
+from typing import TYPE_CHECKING, Dict, Pattern, Set
 
 import tomli
 from robot.utils import FileReader
 
-from robocop.exceptions import (
-    ArgumentFileNotFoundError,
-    CircularArgumentFileError,
-    ConfigGeneralError,
-    InvalidArgumentError,
-    TomlFileNotFoundError,
-)
+from robocop import exceptions
 from robocop.files import DEFAULT_EXCLUDES, find_file_in_project_root, find_project_root
 from robocop.rules import RuleFilter, RuleSeverity
-from robocop.utils import RecommendationFinder
 from robocop.version import __version__
 
+if TYPE_CHECKING:
+    from robocop.rules import Rule
+
 
 def translate_pattern(pattern: str) -> Pattern:
     return re.compile(fnmatch.translate(pattern))
@@ -79,14 +75,14 @@ class CustomArgParser(argparse.ArgumentParser):
         if self.from_cli:
             super().error(message)
         else:
-            raise InvalidArgumentError(message)
+            raise exceptions.InvalidArgumentError(message)
 
 
 def validate_regex(pattern: str) -> Pattern:
     try:
         return re.compile(pattern) if pattern is not None else None
     except re.error:
-        raise ConfigGeneralError(f"Provided regex pattern {pattern} failed to be compiled")
+        raise exceptions.ConfigGeneralError(f"Provided regex pattern {pattern} failed to be compiled")
 
 
 def resolve_relative_path(orig_path, config_dir: Path, ensure_exists: bool):
@@ -121,7 +117,7 @@ class ArgumentFileParser:
                 parsed_args.append(arg)
                 continue
             if not args:  # argumentfile option declared but filename was not provided
-                raise ArgumentFileNotFoundError("") from None
+                raise exceptions.ArgumentFileNotFoundError("") from None
             argfile = args.pop(0)
             argfile_path = Path(argfile)
             if argfile_path.is_file():
@@ -157,7 +153,7 @@ class ArgumentFileParser:
         if config_dir is not None:
             argfile = resolve_relative_path(argfile, config_dir, True)
         if argfile in self.loaded_argument_files:
-            raise CircularArgumentFileError(argfile) from None
+            raise exceptions.CircularArgumentFileError(argfile) from None
         self.loaded_argument_files.add(argfile)
         try:
             with FileReader(argfile) as arg_f:
@@ -173,7 +169,7 @@ class ArgumentFileParser:
                     self.config_from = argfile
                 return args
         except FileNotFoundError:
-            raise ArgumentFileNotFoundError(argfile) from None
+            raise exceptions.ArgumentFileNotFoundError(argfile) from None
 
 
 class Config:
@@ -455,8 +451,7 @@ class Config:
         self.remove_severity()
         self.translate_patterns()
         self.print_config_source()
-        self.validate_rule_names(rules)
-        self.check_deprecations(rules)
+        self.validate_rules_exists_and_not_deprecated(rules)
 
     def print_config_source(self):
         # We can only print after reading all configs, since self.verbose is unknown before we read it from config
@@ -494,7 +489,7 @@ class Config:
             with Path(pyproject_path).open("rb") as fp:
                 config = tomli.load(fp)
         except tomli.TOMLDecodeError as err:
-            raise InvalidArgumentError(f"Failed to decode {str(pyproject_path)}: {err}") from None
+            raise exceptions.InvalidArgumentError(f"Failed to decode {str(pyproject_path)}: {err}") from None
         config = config.get("tool", {}).get("robocop", {})
         if self.parse_toml_to_config(config, config_dir):
             self.config_from = pyproject_path
@@ -505,7 +500,7 @@ class Config:
         if args.config is not None:
             config_path = Path(args.config)
             if not config_path.is_file():
-                raise TomlFileNotFoundError(str(config_path)) from None
+                raise exceptions.TomlFileNotFoundError(str(config_path)) from None
             self.load_pyproject_file(config_path)
         for key, value in dict(**vars(args)).items():
             if key in self.__dict__:
@@ -525,48 +520,15 @@ class Config:
         container.remove(old_key)
         container.add(new_key)
 
-    def validate_rule_names(self, rules):
+    def validate_rules_exists_and_not_deprecated(self, rules: Dict[str, "Rule"]):
         for rule in chain(self.include, self.exclude):
             if rule not in rules:
-                similar = RecommendationFinder().find_similar(rule, rules)
-                raise ConfigGeneralError(f"Provided rule '{rule}' does not exist. {similar}")
-
-    def check_deprecations(self, rules):
-        renamed = {
-            # "old-name": "new-name"
-        }
-        deprecated = {
-            # "rule-name": "deprecation message"
-        }
-        if not (deprecated or renamed):
-            return
-        deprecation_header = "### DEPRECATION WARNING ###"
-        deprecation_footer = "This information will disappear in the next version.\n\n"
-        # get all rules mentioned in include and exclude CLI options
-        mentioned_rules = self.include.union(self.exclude)
-        # add the rules mentioned in configure CLI option
-        mentioned_rules.update(configured.split(":", 1)[0] for configured in self.configure)
-        for rule in mentioned_rules:
-            if rule not in rules:  # reports can also be configured, but we only want rules here
-                continue
-            rule_name = rules[rule].name
-            if rule_name in renamed:  # update warning description to specific case
-                print(
-                    f"{deprecation_header}\n"
-                    f"Rule '{rule_name}' is renamed to '{renamed[rule_name]}'.\n"
-                    f"Update your configuration if you're using the old name. "
-                    f"{deprecation_footer}"
-                )
-                self.replace_in_set(self.include, rule_name, renamed[rule_name])
-                self.replace_in_set(self.exclude, rule_name, renamed[rule_name])
-            if rule_name in deprecated:
-                print(
-                    f"{deprecation_header}\n"
-                    f"Rule '{rule_name}' is deprecated - {deprecated[rule_name]}\n"
-                    f"{deprecation_footer}"
-                )
-
-    def is_rule_enabled(self, rule):
+                raise exceptions.RuleDoesNotExist(rule, rules) from None
+            rule_def = rules[rule]
+            if rule_def.deprecated:
+                rule_def.deprecation_warning()
+
+    def is_rule_enabled(self, rule: "Rule") -> bool:
         if self.is_rule_disabled(rule):
             return False
         if self.include or self.include_patterns:  # if any include pattern, it must match with something
@@ -578,8 +540,8 @@ class Config:
             return False
         return rule.enabled
 
-    def is_rule_disabled(self, rule):
-        if not rule.enabled_in_version:
+    def is_rule_disabled(self, rule: "Rule") -> bool:
+        if rule.deprecated or not rule.enabled_in_version:
             return True
         if rule.severity < self.threshold and not rule.config.get("severity_threshold"):
             return True
@@ -590,7 +552,7 @@ class Config:
                 return True
         return False
 
-    def is_path_ignored(self, path):
+    def is_path_ignored(self, path) -> bool:
         for pattern in self.ignore:
             if path.match(pattern):
                 return True
@@ -642,5 +604,7 @@ class Config:
             elif key == "persistent":
                 self.persistent = value
             else:
-                raise InvalidArgumentError(f"Option '{key}' is not supported in pyproject.toml configuration file.")
+                raise exceptions.InvalidArgumentError(
+                    f"Option '{key}' is not supported in pyproject.toml configuration file."
+                ) from None
         return True
diff --git a/robocop/exceptions.py b/robocop/exceptions.py
index edcd813..031b46a 100644
--- a/robocop/exceptions.py
+++ b/robocop/exceptions.py
@@ -1,3 +1,8 @@
+from typing import TYPE_CHECKING, Dict
+
+if TYPE_CHECKING:
+    from robocop.rules import Rule
+
 import robot.errors
 
 
@@ -84,6 +89,24 @@ class InvalidReportName(ConfigGeneralError):
         super().__init__(msg)
 
 
+class RuleDoesNotExist(ConfigGeneralError):
+    def __init__(self, rule: str, rules: Dict[str, "Rule"]):
+        from robocop.utils import RecommendationFinder
+
+        similar = RecommendationFinder().find_similar(rule, rules)
+        msg = f"Provided rule '{rule}' does not exist. {similar}"
+        super().__init__(msg)
+
+
+class RuleOrReportDoesNotExist(ConfigGeneralError):
+    def __init__(self, rule: str, rules: Dict[str, "Rule"]):
+        from robocop.utils import RecommendationFinder
+
+        similar = RecommendationFinder().find_similar(rule, rules)
+        msg = f"Provided rule or report '{rule}' does not exist. {similar}"
+        super().__init__(msg)
+
+
 class RobotFrameworkParsingError(Exception):
     def __init__(self):
         msg = (
diff --git a/robocop/rules.py b/robocop/rules.py
index 1a50ab0..3ad6b7a 100644
--- a/robocop/rules.py
+++ b/robocop/rules.py
@@ -284,6 +284,7 @@ class Rule:
         docs: str = "",
         added_in_version: Optional[str] = None,
         enabled: bool = True,
+        deprecated: bool = False,
     ):
         """
         :param params: RuleParam() or SeverityThreshold() instances
@@ -295,6 +296,8 @@ class Rule:
         :param docs: Full documentation of the rule (rst supported)
         description of the rule
         :param added_in_version: Version of the Robocop when the Rule was created
+        :param enabled: Enable/disable rule by default using this parameter
+        :param deprecated: Deprecate rule. If rule is used in configuration, it will issue a warning.
         """
         self.rule_id = rule_id
         self.name = name
@@ -321,6 +324,7 @@ class Rule:
         for param in params:
             self.config[param.name] = param
         self.supported_version = version if version else "All"
+        self.deprecated = deprecated
         self.enabled_in_version = self.supported_in_rf_version(version)
         self.added_in_version = added_in_version
         self.community_rule = False
@@ -332,6 +336,8 @@ class Rule:
 
     @property
     def enabled(self):
+        if self.deprecated:
+            return False
         return self.config["enabled"].value
 
     @enabled.setter
@@ -348,6 +354,10 @@ class Rule:
             desc += self.docs
         return desc
 
+    def deprecation_warning(self):
+        """Used when rule is deprecated and used in configuration."""
+        print(f"Rule {self.severity}{self.rule_id} {self.name} is deprecated. " f"Remove it from your configuration.")
+
     def get_severity_with_threshold(self, threshold_value):
         if threshold_value is None:
             return self.severity
@@ -377,10 +387,7 @@ class Rule:
         return self.msg
 
     def __str__(self):
-        return (
-            f"Rule - {self.rule_id} [{self.config['severity'].value}]: {self.name}: {self.msg} "
-            f"({self.get_enabled_status_desc()})"
-        )
+        return f"Rule - {self.rule_id} [{self.severity}]: {self.name}: {self.msg} ({self.get_enabled_status_desc()})"
 
     def get_enabled_status_desc(self):
         s = "enabled" if self.enabled else "disabled"
diff --git a/robocop/run.py b/robocop/run.py
index f5ac06d..be21aea 100644
--- a/robocop/run.py
+++ b/robocop/run.py
@@ -252,18 +252,18 @@ class Robocop:
             if config.count(":") < 2:
                 raise robocop.exceptions.ConfigGeneralError(
                     f"Provided invalid config: '{config}' (general pattern: <rule>:<param>:<value>)"
-                )
+                ) from None
             rule_or_report, param, value = config.split(":", maxsplit=2)
             if rule_or_report in self.rules:
                 rule = self.rules[rule_or_report]
-                rule.configure(param, value)
+                if rule.deprecated:
+                    rule.deprecation_warning()
+                else:
+                    rule.configure(param, value)
             elif rule_or_report in self.reports:
                 self.reports[rule_or_report].configure(param, value)
             else:
-                similar = RecommendationFinder().find_similar(rule_or_report, self.rules)
-                raise robocop.exceptions.ConfigGeneralError(
-                    f"Provided rule or report '{rule_or_report}' does not exist. {similar}"
-                )
+                raise robocop.exceptions.RuleOrReportDoesNotExist(rule_or_report, self.rules)
 
 
 def run_robocop():


Test patch:
diff --git a/tests/test_data/ext_rules/deprecated/deprecated_rules.py b/tests/test_data/ext_rules/deprecated/deprecated_rules.py
new file mode 100644
index 0000000..a2297ca
--- /dev/null
+++ b/tests/test_data/ext_rules/deprecated/deprecated_rules.py
@@ -0,0 +1,24 @@
+from robocop.checkers import VisitorChecker
+from robocop.rules import Rule, RuleSeverity
+
+rules = {
+    "1101": Rule(rule_id="1101", name="not-deprecated", msg="Not deprecated", severity=RuleSeverity.ERROR),
+    "1102": Rule(rule_id="1102", name="deprecated", msg="Deprecated", deprecated=True, severity=RuleSeverity.ERROR),
+    "1103": Rule(
+        rule_id="1103",
+        name="deprecated-no-implementation",
+        msg="Deprecated and without implementation",
+        deprecated=True,
+        severity=RuleSeverity.WARNING,
+    ),
+}
+
+
+class FirstChecker(VisitorChecker):
+    reports = ("not-deprecated", "deprecated")
+
+    def visit_Keyword(self, node):  # noqa
+        self.report("not-deprecated", node=node)
+
+    def visit_TestCase(self, node):  # noqa
+        self.report("deprecated", node=node)
diff --git a/tests/utest/test_external_rules.py b/tests/utest/test_external_rules.py
index 98a5636..1595b5f 100644
--- a/tests/utest/test_external_rules.py
+++ b/tests/utest/test_external_rules.py
@@ -181,3 +181,36 @@ class TestExternalRules:
         robocop_pre_load.check_for_disabled_rules()
         assert robocop_pre_load.rules["1101"].enabled
         assert robocop_pre_load.rules["1102"].enabled
+
+
+class TestDeprecatedRules:
+    def test_load_deprecated_rules(self, robocop_pre_load, capsys):
+        robocop_pre_load.config.ext_rules = {str(TEST_DATA / "deprecated" / "deprecated_rules.py")}
+        robocop_pre_load.reload_config()
+        # Rules loaded correctly
+        assert all(
+            rule in robocop_pre_load.rules for rule in ("not-deprecated", "deprecated", "deprecated-no-implementation")
+        )
+        # Rules enabled status is set correctly
+        assert all(not robocop_pre_load.rules[rule].enabled for rule in ("deprecated", "deprecated-no-implementation"))
+        assert robocop_pre_load.rules["not-deprecated"].enabled
+        # Deprecated rules are disabled, non deprecated are not
+        assert all(
+            not robocop_pre_load.config.is_rule_enabled(robocop_pre_load.rules[rule])
+            for rule in ("deprecated", "deprecated-no-implementation")
+        )
+        assert robocop_pre_load.config.is_rule_enabled(robocop_pre_load.rules["not-deprecated"])
+        # No warning if rule not mentioned in the configuration
+        out, _ = capsys.readouterr()
+        assert "Rule W1103 deprecated-no-implementation is deprecated. Remove it from your configuration.\n" not in out
+        assert "Rule W1102 deprecated is deprecated. Remove it from your configuration.\n" not in out
+
+    def test_use_deprecated_rule(self, robocop_pre_load, capsys):
+        robocop_pre_load.config.ext_rules = {str(TEST_DATA / "deprecated" / "deprecated_rules.py")}
+        robocop_pre_load.config.configure = ["deprecated:enabled:True"]
+        robocop_pre_load.config.include = {"deprecated-no-implementation"}
+        robocop_pre_load.config.exclude = {"deprecated", "not-deprecated"}
+        robocop_pre_load.reload_config()
+        out, _ = capsys.readouterr()
+        assert "Rule W1103 deprecated-no-implementation is deprecated. Remove it from your configuration.\n" in out
+        assert "Rule W1102 deprecated is deprecated. Remove it from your configuration.\n"