Use the dataclass helper and make vectors immutable #106
Conversation
|
Right, this fails on 3.6 because |
|
PS: There is a version of |
|
Add |
AstraLuma
left a comment
AstraLuma
left a comment
There was a problem hiding this comment.
I would love to see a utility method instead of requiring the use of dataclasses.replace()--especially since it doesn't actually replace anything. Some name ideas:
.update().mutate().change()
| return _find_lowest_type(left, right) | ||
|
|
||
|
|
||
| @dataclass(eq=False, frozen=True, init=False, repr=False) |
There was a problem hiding this comment.
This should probably have eq=True for reasons of hashing?
There was a problem hiding this comment.
That should result in a TypeError since Vector2 defines it's own __eq__.
Moreover, I don't think we can make it an hashable type, as AFAIK hash(vector) == hash(vector_like) would need to hold wherever vector == vector_like.
There was a problem hiding this comment.
I'm.... not certain off-hand.
There was a problem hiding this comment.
Yes, for this you want to key the same, two equal things must hash the same.
In this case, I don't think we want to key the same to anything not an actual vector? And this becomes a problem not because we expect users to key dictionaries on vectors, but because of things like functools.lru_cache, which use dictionaries under the hood.
There was a problem hiding this comment.
Yes, it will definitely break anything that expects to index something off a vector-like; it's also a violation of the hash protocol, so I'd really rather not make it hashable.
OK with proceeding without adding __hash__?
|
@astronouth7303 I didn't suggest requiring users to know about |
Mostly a no-op for now.
- Use the float() builtin, instead of calling a dunder directly. Since it raises ValueError, we can clean up the exception handling. - Only do the conversions in the Vector2 constructor; Vector2.convert doesn't need to coerce to float before calling the constructor.
This dependency is optional in Python 3.7 or later, as [PEP 557] made it part of the standard library. [PEP 557]: https://www.python.org/dev/peps/pep-0557/
nicoonoclaste
force-pushed
the
dataclasses
branch
from
e097d6e to
a0d565f
Compare
|
OK, rebased now that CI was green again and that your comments were handled |
2 participants
Following a discussion with @astronouth7303.