Value objects use dataclasses#225
Conversation
| def __init__(self, *module_tails: str, independent: bool = True) -> None: | ||
| object.__setattr__(self, "module_tails", set(module_tails)) | ||
| object.__setattr__(self, "independent", independent) |
There was a problem hiding this comment.
🧀 This custom __init__ needed since module_tails is a variadic argument.
There was a problem hiding this comment.
🧀 object.__setattr__ needed since the dataclass is frozen.
There was a problem hiding this comment.
Might be worth adding these pull request comments as code comments?
CodSpeed Instrumentation Performance ReportMerging #225 will not alter performanceComparing Summary
|
Peter554
force-pushed
the
value-objectgs
branch
2 times, most recently
from
d610d7f to
eee7761
Compare
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Thanks for looking at this - I agree it is better.
I feel if we're going to do it, we should completely remove ValueObject, what do you think?
Would you mind double checking Import Linter's tests pass on this version?
| A Python module. | ||
| """ | ||
|
|
||
| __slots__ = ("name",) |
There was a problem hiding this comment.
We should do slots=True on the dataclass decorator, right?
There was a problem hiding this comment.
I believe slots was only added in 3.10 to the dataclass decorator, and we support 3.9.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@seddonym I've updated the PR now.
I've run the import-linter tests locally (Python 3.12) and they are passing 🤞 |
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
This is nearly there. I would prefer not to drop the slots optimization though.
| A Python module. | ||
| """ | ||
|
|
||
| __slots__ = ("name",) |
There was a problem hiding this comment.
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.)
src/grimp/domain/valueobjects.py
Outdated
| 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'.""" |
There was a problem hiding this comment.
This should be a line comment i.e. # ....
| def __init__(self, *module_tails: str, independent: bool = True) -> None: | ||
| object.__setattr__(self, "module_tails", set(module_tails)) | ||
| object.__setattr__(self, "independent", independent) |
There was a problem hiding this comment.
Might be worth adding these pull request comments as code comments?
|
|
||
|
|
||
| class TestDirectImport: | ||
| def test_repr(self): |
There was a problem hiding this comment.
I think we've lost coverage for the str method, instead of removing this mind tweaking it so we test that instead?
There was a problem hiding this comment.
True - I've changed this to test the __str__ method now.
This is a change of behaviour - see the removed test. However it is consistent with how value objects work in dataclasses / attrs
These tests feel a bit unneceessary now that we're using the dataclasses implementation.
2 participants
Some tidying to adapt
valueobjects.pyto use python@dataclasses. IMO this keeps the code a bit more uniform/standard - I don't think we really need a custom value object implementation.