From b1811041b40d3c779ee987eabe7900a96da950fa Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Mon, 16 Jun 2025 21:07:41 +0200 Subject: [PATCH 1/5] Fix test_non_ascii to use Module instead of raw strings --- tests/unit/adaptors/test_importscanner.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/unit/adaptors/test_importscanner.py b/tests/unit/adaptors/test_importscanner.py index 54d3cccf..f4d1795f 100644 --- a/tests/unit/adaptors/test_importscanner.py +++ b/tests/unit/adaptors/test_importscanner.py @@ -111,20 +111,20 @@ def test_non_ascii(): assert result == { DirectImport( - importer="mypackage.blue", - imported="ñon_ascii_变", + importer=Module("mypackage.blue"), + imported=Module("ñon_ascii_变"), line_number=1, line_contents="from ñon_ascii_变 import *", ), DirectImport( - importer="mypackage.blue", - imported="mypackage.ñon_ascii_变", + importer=Module("mypackage.blue"), + imported=Module("mypackage.ñon_ascii_变"), line_number=2, line_contents="from . import ñon_ascii_变", ), DirectImport( - importer="mypackage.blue", - imported="mypackage.ñon_ascii_变.ラーメン", + importer=Module("mypackage.blue"), + imported=Module("mypackage.ñon_ascii_变.ラーメン"), line_number=3, line_contents="import mypackage.ñon_ascii_变.ラーメン", ), From aaeaf6c629c555b9dc9ad285f5fd43dd00a0cfce Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Mon, 16 Jun 2025 21:08:23 +0200 Subject: [PATCH 2/5] Adapt value objects to use dataclasses --- src/grimp/domain/valueobjects.py | 57 ++++++++++++++------------------ 1 file changed, 24 insertions(+), 33 deletions(-) diff --git a/src/grimp/domain/valueobjects.py b/src/grimp/domain/valueobjects.py index a31468a2..dcbdcb57 100644 --- a/src/grimp/domain/valueobjects.py +++ b/src/grimp/domain/valueobjects.py @@ -1,10 +1,14 @@ -from typing import Any +from dataclasses import dataclass, astuple +from typing import Set, Any +@dataclass(frozen=True, repr=False, eq=False) class ValueObject: def __repr__(self) -> str: return "<{}: {}>".format(self.__class__.__name__, self) + # We use a custom __eq__ method to enforce the liskov principle. + # e.g. SpecialModule("foo") == Module("foo") where SpecialModule is a subclass of Module. def __eq__(self, other: Any) -> bool: if isinstance(other, type(self)) or isinstance(self, type(other)): return hash(self) == hash(other) @@ -12,22 +16,17 @@ def __eq__(self, other: Any) -> bool: return False def __hash__(self) -> int: - return hash(str(self)) + return hash(astuple(self)) +@dataclass(frozen=True, repr=False, eq=False) class Module(ValueObject): """ A Python module. """ - __slots__ = ("name",) - - def __init__(self, name: str) -> None: - """ - Args: - name: The fully qualified name of a Python module, e.g. 'package.foo.bar'. - """ - self.name = name + # The fully qualified name of a Python module, e.g. 'package.foo.bar'. + name: str def __str__(self) -> str: return self.name @@ -61,31 +60,22 @@ def is_descendant_of(self, module: "Module") -> bool: return self.name.startswith(f"{module.name}.") +@dataclass(frozen=True, repr=False, eq=False) class DirectImport(ValueObject): """ An import between one module and another. """ - def __init__( - self, - *, - importer: Module, - imported: Module, - line_number: int, - line_contents: str, - ) -> None: - self.importer = importer - self.imported = imported - self.line_number = line_number - self.line_contents = line_contents + importer: Module + imported: Module + line_number: int + line_contents: str def __str__(self) -> str: - return "{} -> {} (l. {})".format(self.importer, self.imported, self.line_number) - - def __hash__(self) -> int: - return hash((str(self), self.line_contents)) + return f"{self.importer} -> {self.imported} (l. {self.line_number})" +@dataclass(frozen=True, repr=False, eq=False) class Layer(ValueObject): """ A layer within a layered architecture. @@ -94,13 +84,14 @@ class Layer(ValueObject): independent. This is the default. """ - def __init__( - self, - *module_tails: str, - independent: bool = True, - ) -> None: - self.module_tails = set(module_tails) - self.independent = independent + module_tails: Set[str] + independent: bool + + # A custom `__init__` is needed since `module_tails` is a variadic argument. + def __init__(self, *module_tails: str, independent: bool = True) -> None: + # `object.__setattr__` is needed since the dataclass is frozen. + object.__setattr__(self, "module_tails", set(module_tails)) + object.__setattr__(self, "independent", independent) def __str__(self) -> str: return f"{self.module_tails}, independent={self.independent}" From c1cad93cb10b313057fd399c1513da82da873663 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Mon, 16 Jun 2025 21:09:23 +0200 Subject: [PATCH 3/5] Remove custom __eq__ for value objects This is a change of behaviour - see the removed test. However it is consistent with how value objects work in dataclasses / attrs --- src/grimp/domain/valueobjects.py | 23 ++++++----------------- tests/unit/domain/test_valueobjects.py | 10 ---------- 2 files changed, 6 insertions(+), 27 deletions(-) diff --git a/src/grimp/domain/valueobjects.py b/src/grimp/domain/valueobjects.py index dcbdcb57..95d2d31f 100644 --- a/src/grimp/domain/valueobjects.py +++ b/src/grimp/domain/valueobjects.py @@ -1,25 +1,14 @@ -from dataclasses import dataclass, astuple -from typing import Set, Any +from dataclasses import dataclass +from typing import Set -@dataclass(frozen=True, repr=False, eq=False) +@dataclass(frozen=True, repr=False) class ValueObject: def __repr__(self) -> str: return "<{}: {}>".format(self.__class__.__name__, self) - # We use a custom __eq__ method to enforce the liskov principle. - # e.g. SpecialModule("foo") == Module("foo") where SpecialModule is a subclass of Module. - def __eq__(self, other: Any) -> bool: - if isinstance(other, type(self)) or isinstance(self, type(other)): - return hash(self) == hash(other) - else: - return False - - def __hash__(self) -> int: - return hash(astuple(self)) - -@dataclass(frozen=True, repr=False, eq=False) +@dataclass(frozen=True, repr=False) class Module(ValueObject): """ A Python module. @@ -60,7 +49,7 @@ def is_descendant_of(self, module: "Module") -> bool: return self.name.startswith(f"{module.name}.") -@dataclass(frozen=True, repr=False, eq=False) +@dataclass(frozen=True, repr=False) class DirectImport(ValueObject): """ An import between one module and another. @@ -75,7 +64,7 @@ def __str__(self) -> str: return f"{self.importer} -> {self.imported} (l. {self.line_number})" -@dataclass(frozen=True, repr=False, eq=False) +@dataclass(frozen=True, repr=False) class Layer(ValueObject): """ A layer within a layered architecture. diff --git a/tests/unit/domain/test_valueobjects.py b/tests/unit/domain/test_valueobjects.py index d63d0489..b699fe3c 100644 --- a/tests/unit/domain/test_valueobjects.py +++ b/tests/unit/domain/test_valueobjects.py @@ -18,16 +18,6 @@ def test_equals(self): # Also non-Module instances should not be treated as equal. assert a != "foo" - def test_equals_obeys_liskov(self): - class SpecialModule(Module): - pass - - module = Module("foo.bar") - special_module = SpecialModule("foo.bar") - - assert module == special_module - assert special_module == module - def test_hash(self): a = Module("foo.bar") b = Module("foo.bar") From bd5edffea7294ef6a1bc44fbd5ab14ad2042a950 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 18 Jun 2025 19:10:08 +0200 Subject: [PATCH 4/5] Remove ValueObject base class --- src/grimp/domain/valueobjects.py | 18 ++++++------------ tests/unit/domain/test_valueobjects.py | 8 ++++---- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/grimp/domain/valueobjects.py b/src/grimp/domain/valueobjects.py index 95d2d31f..63145f17 100644 --- a/src/grimp/domain/valueobjects.py +++ b/src/grimp/domain/valueobjects.py @@ -2,14 +2,8 @@ from typing import Set -@dataclass(frozen=True, repr=False) -class ValueObject: - def __repr__(self) -> str: - return "<{}: {}>".format(self.__class__.__name__, self) - - -@dataclass(frozen=True, repr=False) -class Module(ValueObject): +@dataclass(frozen=True) +class Module: """ A Python module. """ @@ -49,8 +43,8 @@ def is_descendant_of(self, module: "Module") -> bool: return self.name.startswith(f"{module.name}.") -@dataclass(frozen=True, repr=False) -class DirectImport(ValueObject): +@dataclass(frozen=True) +class DirectImport: """ An import between one module and another. """ @@ -64,8 +58,8 @@ def __str__(self) -> str: return f"{self.importer} -> {self.imported} (l. {self.line_number})" -@dataclass(frozen=True, repr=False) -class Layer(ValueObject): +@dataclass(frozen=True) +class Layer: """ A layer within a layered architecture. diff --git a/tests/unit/domain/test_valueobjects.py b/tests/unit/domain/test_valueobjects.py index b699fe3c..8448eee0 100644 --- a/tests/unit/domain/test_valueobjects.py +++ b/tests/unit/domain/test_valueobjects.py @@ -4,9 +4,9 @@ class TestModule: - def test_repr(self): + def test_str(self): module = Module("foo.bar") - assert repr(module) == "" + assert str(module) == "foo.bar" def test_equals(self): a = Module("foo.bar") @@ -43,14 +43,14 @@ def test_parent(self, module, expected): class TestDirectImport: - def test_repr(self): + def test_str(self): import_path = DirectImport( importer=Module("foo"), imported=Module("bar"), line_number=10, line_contents="import bar", ) - assert repr(import_path) == " bar (l. 10)>" + assert str(import_path) == "foo -> bar (l. 10)" def test_equals(self): a = DirectImport( From 81dc0bfd7a431f617229a98b953b9ec0abff43c7 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Thu, 19 Jun 2025 12:14:21 +0200 Subject: [PATCH 5/5] Remove tests for eq and hash These tests feel a bit unneceessary now that we're using the dataclasses implementation. --- tests/unit/domain/test_valueobjects.py | 108 ------------------------- 1 file changed, 108 deletions(-) diff --git a/tests/unit/domain/test_valueobjects.py b/tests/unit/domain/test_valueobjects.py index 8448eee0..39448b4c 100644 --- a/tests/unit/domain/test_valueobjects.py +++ b/tests/unit/domain/test_valueobjects.py @@ -8,24 +8,6 @@ def test_str(self): module = Module("foo.bar") assert str(module) == "foo.bar" - def test_equals(self): - a = Module("foo.bar") - b = Module("foo.bar") - c = Module("foo.bar.baz") - - assert a == b - assert a != c - # Also non-Module instances should not be treated as equal. - assert a != "foo" - - def test_hash(self): - a = Module("foo.bar") - b = Module("foo.bar") - c = Module("foo.bar.baz") - - assert hash(a) == hash(b) - assert hash(a) != hash(c) - @pytest.mark.parametrize( "module, expected", ( @@ -51,93 +33,3 @@ def test_str(self): line_contents="import bar", ) assert str(import_path) == "foo -> bar (l. 10)" - - def test_equals(self): - a = DirectImport( - importer=Module("foo"), - imported=Module("bar"), - line_number=10, - line_contents="import bar", - ) - b = DirectImport( - importer=Module("foo"), - imported=Module("bar"), - line_number=10, - line_contents="import bar", - ) - c = DirectImport( - importer=Module("foo"), - imported=Module("baz"), - line_number=10, - line_contents="import bar", - ) - d = DirectImport( - importer=Module("foobar"), - imported=Module("bar"), - line_number=10, - line_contents="import bar", - ) - e = DirectImport( - importer=Module("foo"), - imported=Module("bar"), - line_number=11, - line_contents="import bar", - ) - f = DirectImport( - importer=Module("foo"), - imported=Module("bar"), - line_number=10, - line_contents="from . import bar", - ) - - assert a == b - assert a != c - assert a != d - assert a != e - assert a != f - # Also non-DirectImport instances should not be treated as equal. - assert a != "foo" - - def test_hash(self): - a = DirectImport( - importer=Module("foo"), - imported=Module("bar"), - line_number=10, - line_contents="import bar", - ) - b = DirectImport( - importer=Module("foo"), - imported=Module("bar"), - line_number=10, - line_contents="import bar", - ) - c = DirectImport( - importer=Module("foo"), - imported=Module("baz"), - line_number=10, - line_contents="import bar", - ) - d = DirectImport( - importer=Module("foobar"), - imported=Module("bar"), - line_number=10, - line_contents="import bar", - ) - e = DirectImport( - importer=Module("foo"), - imported=Module("bar"), - line_number=11, - line_contents="import bar", - ) - f = DirectImport( - importer=Module("foo"), - imported=Module("bar"), - line_number=10, - line_contents="from . import bar", - ) - - assert hash(a) == hash(b) - assert hash(a) != hash(c) - assert hash(a) != hash(d) - assert hash(a) != hash(e) - assert hash(a) != hash(f)