Repository: MarketSquare/robotframework-robocop (python)

Issue:
Could the `packaging` dependency be dropped?
From what I can see, packaging is used just for its `Version` and it checks mostly the major/minor versions, where getting the major/minor version through a split with `.` and using it as a tuple only with major/minor should suffice.

This dep brings or `pyparsing` too, so, it brings up a lot of code just to check the version...

As for the reason for me, sometimes I do have issues because a version of `robocop` is vendored in the language server, which means it's not pip-installed, but if the user has an older version of `packaging` installed in its env `robocop` doesn't really work...

Pull request:
Remove packaging
This PR removes packaging dependency from our repo. It's because we have a lot of problems with this dependency, and it introduces a lot of interdependencies we do need. 

The code was copied from packaging.version and them trimmed and simplified until it matched our needs. I will review it once again soon to see if we can afford to cut it more (hence draft state).

Closes #742

Patch:
diff --git a/docs/releasenotes/3.0.0.rst b/docs/releasenotes/3.0.0.rst
index 6c50bf5..9e9e24f 100644
--- a/docs/releasenotes/3.0.0.rst
+++ b/docs/releasenotes/3.0.0.rst
@@ -31,7 +31,16 @@ Bumped dependencies (#792)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 There were some compatibility issues due to outdated packages.
-Now, all of them are updated, especially `packaging` now allows to use v23.
+Now, all of them are updated.
+
+Removed packaging dependency (#742)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+``packaging`` dependency brings a lot of other dependencies and often caused
+compatibility issues for us. We only used it for its versioning module when
+deciding if the Robocop rule should be enabled for given Robot Framework rule.
+That's why we have decided to remove the dependency and rewrite version handling
+in the Roocop.
 
 Improved internal quality (#652)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/robocop/rules.py b/robocop/rules.py
index c5dd356..a5ea11f 100644
--- a/robocop/rules.py
+++ b/robocop/rules.py
@@ -29,10 +29,10 @@ from textwrap import dedent
 from typing import Any, Callable, Dict, Optional, Pattern, Union
 
 from jinja2 import Template
-from packaging.specifiers import SpecifierSet
 
 import robocop.exceptions
 from robocop.utils import ROBOT_VERSION
+from robocop.utils.version_matching import VersionSpecifier
 
 
 @total_ordering
@@ -304,7 +304,7 @@ class Rule:
     def supported_in_rf_version(version: str) -> bool:
         if not version:
             return True
-        return ROBOT_VERSION in SpecifierSet(version, prereleases=True)
+        return ROBOT_VERSION in VersionSpecifier(version)
 
     @staticmethod
     def get_template(msg: str) -> Optional[Template]:
diff --git a/robocop/utils/misc.py b/robocop/utils/misc.py
index a5d4f31..e408f9c 100644
--- a/robocop/utils/misc.py
+++ b/robocop/utils/misc.py
@@ -8,21 +8,20 @@ from pathlib import Path
 from typing import Dict, List, Optional, Pattern, Tuple
 
 from robot.api import Token
-from robot.parsing.model.statements import EmptyLine
 
 try:
     from robot.api.parsing import Variable
 except ImportError:
     from robot.parsing.model.statements import Variable
 
-from packaging import version
 from robot.version import VERSION as RF_VERSION
 
 from robocop.exceptions import InvalidExternalCheckerError
+from robocop.utils.version_matching import Version
 from robocop.version import __version__
 
-ROBOT_VERSION = version.parse(RF_VERSION)
-ROBOT_WITH_LANG = version.parse("6.0")
+ROBOT_VERSION = Version(RF_VERSION)
+ROBOT_WITH_LANG = Version("6.0")
 
 
 def rf_supports_lang():
diff --git a/robocop/utils/version_matching.py b/robocop/utils/version_matching.py
new file mode 100644
index 0000000..3718bc7
--- /dev/null
+++ b/robocop/utils/version_matching.py
@@ -0,0 +1,395 @@
+import collections
+import itertools
+import re
+from functools import total_ordering
+from typing import List, Optional, SupportsInt, Tuple, Union
+
+VERSION_PATTERN = r"""
+    v?
+    (?:
+        (?P<release>[0-9]+(?:\.[0-9]+)*)                  # release segment
+        (?P<pre>                                          # pre-release
+            [-_\.]?
+            (?P<pre_l>(a|b|c|rc|alpha|beta|pre|preview))
+            [-_\.]?
+            (?P<pre_n>[0-9]+)?
+        )?
+        (?P<dev>                                          # dev release
+            [-_\.]?
+            (?P<dev_l>dev)
+            [-_\.]?
+            (?P<dev_n>[0-9]+)?
+        )?
+    )
+    (?:\+(?P<local>[a-z0-9]+(?:[-_\.][a-z0-9]+)*))?       # local version
+"""
+
+
+class InfinityType:
+    def __lt__(self, other: object) -> bool:
+        return False
+
+    def __le__(self, other: object) -> bool:
+        return False
+
+    def __eq__(self, other: object) -> bool:
+        return isinstance(other, self.__class__)
+
+    def __gt__(self, other: object) -> bool:
+        return True
+
+    def __ge__(self, other: object) -> bool:
+        return True
+
+    def __neg__(self: object) -> "NegativeInfinityType":
+        return NegativeInfinity
+
+
+class NegativeInfinityType:
+    def __lt__(self, other: object) -> bool:
+        return True
+
+    def __le__(self, other: object) -> bool:
+        return True
+
+    def __eq__(self, other: object) -> bool:
+        return isinstance(other, self.__class__)
+
+    def __gt__(self, other: object) -> bool:
+        return False
+
+    def __ge__(self, other: object) -> bool:
+        return False
+
+    def __neg__(self: object) -> InfinityType:
+        return Infinity
+
+
+Infinity = InfinityType()
+NegativeInfinity = NegativeInfinityType()
+
+
+def _get_comparison_key(
+    release: Tuple[int, ...],
+    pre: Optional[Tuple[str, int]],
+    dev: Optional[Tuple[str, int]],
+):
+    # When we compare a release version, we want to compare it with all the
+    # trailing zeros removed. So we'll use a reverse the list, drop all the now
+    # leading zeros until we come to something non zero, then take the rest
+    # re-reverse it back into the correct order and make it a tuple and use
+    # that for our sorting key.
+    _release = tuple(reversed(list(itertools.dropwhile(lambda x: x == 0, reversed(release)))))
+
+    # We need to "trick" the sorting algorithm to put 1.0.dev0 before 1.0a0.
+    # We'll do this by abusing the pre segment, but we _only_ want to do this
+    # if there is not a pre segment. Otherwise, the normal sorting rules will handle this case correctly.
+    if pre is None and dev is not None:
+        _pre = NegativeInfinity
+    # Versions without a pre-release (except as noted above) should sort after
+    # those with one.
+    elif pre is None:
+        _pre = Infinity
+    else:
+        _pre = pre
+
+    # Versions without a development segment should sort after those with one.
+    _dev = Infinity if dev is None else dev
+    return _release, _pre, _dev
+
+
+def _parse_letter_version(letter: str, number: Union[str, bytes, SupportsInt]) -> Optional[Tuple[str, int]]:
+    if letter:
+        # We consider there to be an implicit 0 in a pre-release if there is
+        # not a numeral associated with it.
+        if number is None:
+            number = 0
+
+        # We normalize any letters to their lower case form
+        letter = letter.lower()
+
+        if letter == "alpha":
+            letter = "a"
+        elif letter == "beta":
+            letter = "b"
+        elif letter in ["c", "pre", "preview"]:
+            letter = "rc"
+
+        return letter, int(number)
+    return None
+
+
+@total_ordering
+class Version:
+    _version_pattern = re.compile(rf"^\s*{VERSION_PATTERN}\s*$", re.VERBOSE | re.IGNORECASE)
+
+    def __init__(self, version: str) -> None:
+        match = self._version_pattern.search(version)
+        self.release = tuple(int(i) for i in match.group("release").split("."))
+        self.pre = _parse_letter_version(match.group("pre_l"), match.group("pre_n"))
+        self._dev = _parse_letter_version(match.group("dev_l"), match.group("dev_n"))
+        self._key = _get_comparison_key(
+            self.release,
+            self.pre,
+            self._dev,
+        )
+
+    def __lt__(self, other) -> bool:
+        return self._key < other._key
+
+    def __eq__(self, other) -> bool:
+        return self._key == other._key
+
+    def __str__(self) -> str:
+        parts = [".".join(str(x) for x in self.release)]
+        if self.pre is not None:
+            parts.append("".join(str(x) for x in self.pre))
+
+        if self.dev is not None:
+            parts.append(f".dev{self.dev}")
+
+        return "".join(parts)
+
+    @property
+    def dev(self) -> Optional[int]:
+        return self._dev[1] if self._dev else None
+
+    @property
+    def public(self) -> str:
+        return str(self).split("+", 1)[0]
+
+    @property
+    def base_version(self) -> str:
+        parts = [".".join(str(x) for x in self.release)]
+        return "".join(parts)
+
+    @property
+    def is_prerelease(self) -> bool:
+        return self.dev is not None or self.pre is not None
+
+    @property
+    def major(self) -> int:
+        return self.release[0] if len(self.release) >= 1 else 0
+
+    @property
+    def minor(self) -> int:
+        return self.release[1] if len(self.release) >= 2 else 0
+
+    @property
+    def micro(self) -> int:
+        return self.release[2] if len(self.release) >= 3 else 0
+
+
+def _pad_version(left, right):
+    left_split, right_split = [], []
+
+    # Get the release segment of our versions
+    left_split.append(list(itertools.takewhile(lambda x: x.isdigit(), left)))
+    right_split.append(list(itertools.takewhile(lambda x: x.isdigit(), right)))
+
+    # Get the rest of our versions
+    left_split.append(left[len(left_split[0]) :])
+    right_split.append(right[len(right_split[0]) :])
+
+    # Insert our padding
+    left_split.insert(1, ["0"] * max(0, len(right_split[0]) - len(left_split[0])))
+    right_split.insert(1, ["0"] * max(0, len(left_split[0]) - len(right_split[0])))
+
+    return (list(itertools.chain(*left_split)), list(itertools.chain(*right_split)))
+
+
+_prefix_regex = re.compile(r"^([0-9]+)((?:a|b|c|rc)[0-9]+)$")
+
+
+def _version_split(version: str):
+    result: List[str] = []
+    for item in version.split("."):
+        match = _prefix_regex.search(item)
+        if match:
+            result.extend(match.groups())
+        else:
+            result.append(item)
+    return result
+
+
+def _is_not_suffix(segment: str) -> bool:
+    return not any(segment.startswith(prefix) for prefix in ("dev", "a", "b", "rc"))
+
+
+class VersionSpecifier:
+    _regex_str = r"""
+        (?P<operator>(~=|==|!=|<=|>=|<|>))
+        (?P<version>
+            (?:
+                # The (non)equality operators allow for wild card and local
+                # versions to be specified so we have to define these two
+                # operators separately to enable that.
+                (?<===|!=)            # Only match for equals and not equals
+
+                \s*
+                v?
+                [0-9]+(?:\.[0-9]+)*   # release
+                (?:                   # pre release
+                    [-_\.]?
+                    (a|b|c|rc|alpha|beta|pre|preview)
+                    [-_\.]?
+                    [0-9]*
+                )?
+                # You cannot use a wild card and a dev or local version
+                # together so group them with a | and make them optional.
+                (?:
+                    (?:[-_\.]?dev[-_\.]?[0-9]*)?         # dev release
+                    (?:\+[a-z0-9]+(?:[-_\.][a-z0-9]+)*)? # local
+                    |
+                    \.\*  # Wild card syntax of .*
+                )?
+            )
+            |
+            (?:
+                # The compatible operator requires at least two digits in the
+                # release segment.
+                (?<=~=)               # Only match for the compatible operator
+
+                \s*
+                v?
+                [0-9]+(?:\.[0-9]+)+   # release  (We have a + instead of a *)
+                (?:                   # pre release
+                    [-_\.]?
+                    (a|b|c|rc|alpha|beta|pre|preview)
+                    [-_\.]?
+                    [0-9]*
+                )?
+                (?:[-_\.]?dev[-_\.]?[0-9]*)?          # dev release
+            )
+            |
+            (?:
+                # All other operators only allow a sub set of what the
+                # (non)equality operators do. Specifically they do not allow
+                # local versions to be specified nor do they allow the prefix
+                # matching wild cards.
+                (?<!==|!=|~=)         # We have special cases for these
+                                      # operators so we want to make sure they
+                                      # don't match here.
+
+                \s*
+                v?
+                [0-9]+(?:\.[0-9]+)*   # release
+                (?:                   # pre release
+                    [-_\.]?
+                    (a|b|c|rc|alpha|beta|pre|preview)
+                    [-_\.]?
+                    [0-9]*
+                )?
+                (?:[-_\.]?dev[-_\.]?[0-9]*)?          # dev release
+            )
+        )
+        """
+
+    _regex = re.compile(r"^\s*" + _regex_str + r"\s*$", re.VERBOSE | re.IGNORECASE)
+
+    _operators = {
+        "~=": "compatible",
+        "==": "equal",
+        "!=": "not_equal",
+        "<=": "less_than_equal",
+        ">=": "greater_than_equal",
+        "<": "less_than",
+        ">": "greater_than",
+    }
+
+    def __init__(self, spec: str = "") -> None:
+        match = self._regex.search(spec)
+        if not match:
+            raise ValueError(f"Invalid specifier: '{spec}'")
+
+        self._spec: Tuple[str, str] = (
+            match.group("operator").strip(),
+            match.group("version").strip(),
+        )
+
+    def __contains__(self, item: str) -> bool:
+        normalized_item = self._coerce_version(item)
+
+        operator_callable = self._get_operator(self.operator)
+        return operator_callable(normalized_item, self.version)
+
+    def _coerce_version(self, version):
+        if not isinstance(version, Version):
+            version = Version(version)
+        return version
+
+    def _get_operator(self, op: str):
+        operator_callable = getattr(self, f"_compare_{self._operators[op]}")
+        return operator_callable
+
+    @property
+    def operator(self) -> str:
+        return self._spec[0]
+
+    @property
+    def version(self) -> str:
+        return self._spec[1]
+
+    def _compare_compatible(self, prospective, spec: str) -> bool:
+        # Compatible releases have an equivalent combination of >= and ==. That
+        # is that ~=2.2 is equivalent to >=2.2,==2.*. This allows us to
+        # implement this in terms of the other specifiers instead of
+        # implementing it ourselves. The only thing we need to do is construct
+        # the other specifiers.
+
+        # We want everything but the last item in the version, but we want to
+        # ignore suffix segments.
+        prefix = ".".join(list(itertools.takewhile(_is_not_suffix, _version_split(spec)))[:-1])
+
+        # Add the prefix notation to the end of our string
+        prefix += ".*"
+
+        return self._get_operator(">=")(prospective, spec) and self._get_operator("==")(prospective, prefix)
+
+    def _compare_equal(self, prospective, spec: str) -> bool:
+        if spec.endswith(".*"):
+            # In the case of prefix matching we want to ignore local segment.
+            prospective = Version(prospective.public)
+            # Split the spec out by dots, and pretend that there is an implicit
+            # dot in between a release segment and a pre-release segment.
+            split_spec = _version_split(spec[:-2])  # Remove the trailing .*
+
+            # Split the prospective version out by dots, and pretend that there
+            # is an implicit dot in between a release segment and a pre-release
+            # segment.
+            split_prospective = _version_split(str(prospective))
+
+            # Shorten the prospective version to be the same length as the spec
+            # so that we can determine if the specifier is a prefix of the
+            # prospective version or not.
+            shortened_prospective = split_prospective[: len(split_spec)]
+
+            # Pad out our two sides with zeros so that they both equal the same
+            # length.
+            padded_spec, padded_prospective = _pad_version(split_spec, shortened_prospective)
+
+            return padded_prospective == padded_spec
+        else:
+            # Convert our spec string into a Version
+            spec_version = Version(spec)
+            return prospective == spec_version
+
+    def _compare_not_equal(self, prospective, spec: str) -> bool:
+        return not self._compare_equal(prospective, spec)
+
+    def _compare_less_than_equal(self, prospective, spec: str) -> bool:
+        return Version(prospective.public) <= Version(spec)
+
+    def _compare_greater_than_equal(self, prospective, spec: str) -> bool:
+        return Version(prospective.public) >= Version(spec)
+
+    def _compare_less_than(self, prospective, spec_str: str) -> bool:
+        spec = Version(spec_str)
+        if not prospective < spec:
+            return False
+        if not spec.is_prerelease and prospective.is_prerelease:
+            return Version(prospective.base_version) != Version(spec.base_version)
+        return True
+
+    def _compare_greater_than(self, prospective, spec_str: str) -> bool:
+        return prospective <= Version(spec_str)
diff --git a/setup.py b/setup.py
index 37733ef..56f8cee 100644
--- a/setup.py
+++ b/setup.py
@@ -54,7 +54,6 @@ setup(
     install_requires=[
         "jinja2>=3.0,<4.0",
         "robotframework>=3.2.2",
-        "packaging>=21,<=23",
         "pathspec>=0.9,<0.12",
         "tomli==1.2.3; python_version < '3.7.0'",
         "tomli>=2.0.0; python_version >= '3.7.0'",


Test patch:
diff --git a/tests/atest/utils/__init__.py b/tests/atest/utils/__init__.py
index ddb20fc..7041c92 100644
--- a/tests/atest/utils/__init__.py
+++ b/tests/atest/utils/__init__.py
@@ -7,11 +7,11 @@ from pathlib import Path
 from typing import List, Optional
 
 import pytest
-from packaging.specifiers import Specifier
 
 from robocop import Robocop
 from robocop.config import Config
 from robocop.utils.misc import ROBOT_VERSION
+from robocop.utils.version_matching import VersionSpecifier
 
 
 @contextlib.contextmanager
@@ -147,4 +147,4 @@ class RuleAcceptance:
     def enabled_in_version(target_version):
         if target_version is None:
             return True
-        return ROBOT_VERSION in Specifier(target_version)
+        return ROBOT_VERSION in VersionSpecifier(target_version)
diff --git a/tests/e2e/test_e2e.py b/tests/e2e/test_e2e.py
index ea609fb..3f17e84 100644
--- a/tests/e2e/test_e2e.py
+++ b/tests/e2e/test_e2e.py
@@ -5,7 +5,6 @@ from pathlib import Path
 from unittest import mock
 
 import pytest
-from packaging import version
 
 import robocop.exceptions
 from robocop.config import Config
@@ -19,6 +18,7 @@ from robocop.exceptions import (
 from robocop.rules import RuleSeverity
 from robocop.run import Robocop
 from robocop.utils.misc import ROBOT_VERSION, rf_supports_lang
+from robocop.utils.version_matching import Version
 
 
 @pytest.fixture
@@ -231,7 +231,7 @@ class TestE2E:
         robocop_instance.run()
         assert not robocop_instance.reports["json_report"].issues
 
-    @pytest.mark.skipif(ROBOT_VERSION > version.parse("4.0"), reason="Error occurs only in RF < 5")
+    @pytest.mark.skipif(ROBOT_VERSION > Version("4.0"), reason="Error occurs only in RF < 5")
     def test_handling_error_in_robot_module(self):
         config = Config()
         test_file = INVALID_TEST_DATA / "invalid_syntax" / "invalid_file.robot"
diff --git a/tests/utest/test_utils.py b/tests/utest/test_utils.py
index 30ed78c..007ca5b 100644
--- a/tests/utest/test_utils.py
+++ b/tests/utest/test_utils.py
@@ -9,6 +9,7 @@ from robocop.utils import (
     pattern_type,
     remove_robot_vars,
 )
+from robocop.utils.version_matching import Version, VersionSpecifier
 
 
 def detect_from_file(file):
@@ -190,3 +191,68 @@ class TestMisc:
         with pytest.raises(ValueError) as error:
             pattern_type(r"[\911]")
         assert r"Invalid regex pattern: bad escape \\9 at position 1" in str(error)
+
+
+@pytest.mark.parametrize(
+    "robot_version, rule_version, should_pass",
+    [
+        ("6.0.2", ">=6", True),
+        ("6.1a2.dev1", ">=6", True),
+        ("6.0rc2.dev1", ">=6", False),
+        ("6.0pre2.dev1", ">=6", False),
+        ("6.0.2", "<5", False),
+        ("6.1a2.dev1", "<=5", False),
+        ("6.1a2.dev1", "~=5.0", False),
+        ("6.0rc2.dev1", "<=6", True),
+        ("6.0rc2.dev", "<=6", True),
+        ("5.0", "<=5dev1", False),
+        ("5.0", ">5dev1", False),
+        ("5.0", ">5.0.dev1", False),
+        ("6.0dev0", ">=6", False),
+        ("6.0", ">=6", True),
+        ("6", ">=6", True),
+        ("5.0.2", ">=6", False),
+        ("5.1b2.dev1", ">=6", False),
+        ("5.1alpha2.dev1", ">=6", False),
+        ("5.1beta2.dev1", "<6", True),
+        ("5.1beta2.dev1", "!=6", True),
+        ("6.0.2", "<4.0", False),
+        ("3.2.1", "<4.0", True),
+        ("5.0", "==5", True),
+        ("5.1", "==5", False),
+        ("4.0", "==4.*", True),
+        ("4.0", "~=4.0", True),
+        ("4.0", "~=5.0", False),
+        ("4.0", "!=4", False),
+        ("4.0", "!=5", True),
+    ],
+)
+def test_version_specifier(robot_version, rule_version, should_pass):
+    rf_version = Version(robot_version)
+    rule_version_spec = VersionSpecifier(rule_version)
+    assert (str(rf_version) in rule_version_spec) == should_pass
+
+
+def test_version_parsing_and_comparison():
+    versions = [Version("4"), Version("4.0"), Version("4.0.0"), Version("4.0.0dev1")]
+    equal_version = Version("4")
+    higher_version = Version("5")
+    lower_version = Version("3")
+    for version in versions:
+        assert (version.major, version.minor, version.micro) == (4, 0, 0)
+        if version.dev is None:
+            assert version == equal_version
+            assert version >= equal_version
+        else:
+            assert version != equal_version
+            assert not version >= equal_version
+        assert version <= equal_version
+        assert version < higher_version
+        assert not version > higher_version
+        assert version > lower_version
+        assert not version < lower_version
+
+
+def test_invalid_version_specifier():
+    with pytest.raises(ValueError, match="Invalid specifier: '=<5'"):
+        VersionSpecifier("=<5")