Skip to content

Value objects use dataclasses #225

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.

Sign up for GitHub

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

Merged
seddonym merged 5 commits into from
Jun 19, 2025
Merged

Value objects use dataclasses #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b181104
Fix test_non_ascii to use Module instead of raw strings
Peter554 Jun 16, 2025
aaeaf6c
Adapt value objects to use dataclasses
Peter554 Jun 16, 2025
c1cad93
Remove custom __eq__ for value objects
Peter554 Jun 16, 2025
bd5edff
Remove ValueObject base class
Peter554 Jun 18, 2025
81dc0bf
Remove tests for eq and hash
Peter554 Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 23 additions & 49 deletions src/grimp/domain/valueobjects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,15 @@
from typing import Any
from dataclasses import dataclass
from typing import Set


class ValueObject:
def __repr__(self) -> str:
return "<{}: {}>".format(self.__class__.__name__, self)

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(str(self))


class Module(ValueObject):
@dataclass(frozen=True)
class Module:
"""
A Python module.
"""

__slots__ = ("name",)
Copy link
Collaborator

@seddonym seddonym Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do slots=True on the dataclass decorator, right?

Copy link
Collaborator Author

@Peter554 Peter554 Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe slots was only added in 3.10 to the dataclass decorator, and we support 3.9.

Copy link
Collaborator

@seddonym seddonym Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh fair enough. Ok to leave this line in then, with a comment to switch once we drop 3.9? (I'm assuming it will work with dataclasses.)

Copy link
Collaborator Author

@Peter554 Peter554 Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing this error when leaving the __slots__ = ("name",) line in

joblib.externals.loky.process_executor.BrokenProcessPool: A task has failed to un-serialize. Please ensure that the arguments of the function are all picklable.

So it seems like it doesn't work...

I suppose since the benchmarks are still passing then the performance change to use slots=True can't be so big.

Copy link
Collaborator

@seddonym seddonym Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's strange, oh well - soon joblib will be gone I hope.

I seem to remember slots making a noticeable difference to some early profiling I did, but as you say, this is what benchmarks are for! So let's drop it for now.


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
Expand Down Expand Up @@ -61,46 +43,38 @@ def is_descendant_of(self, module: "Module") -> bool:
return self.name.startswith(f"{module.name}.")


class DirectImport(ValueObject):
@dataclass(frozen=True)
class DirectImport:
"""
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)
return f"{self.importer} -> {self.imported} (l. {self.line_number})"

def __hash__(self) -> int:
return hash((str(self), self.line_contents))


class Layer(ValueObject):
@dataclass(frozen=True)
class Layer:
"""
A layer within a layered architecture.

If layer.independent is True then the modules within the layer are considered
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}"
12 changes: 6 additions & 6 deletions tests/unit/adaptors/test_importscanner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_变.ラーメン",
),
Expand Down
126 changes: 4 additions & 122 deletions tests/unit/domain/test_valueobjects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,37 +4,9 @@


class TestModule:
def test_repr(self):
def test_str(self):
module = Module("foo.bar")
assert repr(module) == "<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_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")
c = Module("foo.bar.baz")

assert hash(a) == hash(b)
assert hash(a) != hash(c)
assert str(module) == "foo.bar"

@pytest.mark.parametrize(
"module, expected",
Expand All @@ -53,101 +25,11 @@ def test_parent(self, module, expected):


class TestDirectImport:
def test_repr(self):
Copy link
Collaborator

@seddonym seddonym Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've lost coverage for the str method, instead of removing this mind tweaking it so we test that instead?

Copy link
Collaborator Author

@Peter554 Peter554 Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True - I've changed this to test the __str__ method now.

def test_str(self):
import_path = DirectImport(
importer=Module("foo"),
imported=Module("bar"),
line_number=10,
line_contents="import bar",
)
assert repr(import_path) == "<DirectImport: 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)
assert str(import_path) == "foo -> bar (l. 10)"
Loading