Skip to content

Conversation

@dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Jun 11, 2018

Treating Python message classes as data (same as in C++) and implementing __eq__ and __hash__ based on all the fields.

CI for Linux only: Build Status

@dirk-thomas dirk-thomas added enhancement New feature or request in review Waiting for review (Kanban column) labels Jun 11, 2018
@dirk-thomas dirk-thomas self-assigned this Jun 11, 2018
@dhood
Copy link
Member

dhood commented Jun 11, 2018

can you clarify the context/motivation for this?

@dirk-thomas
Copy link
Member Author

Without two message instances can't easily be compared if they contain the same values. The user would need to compare each and every field (as well as recursive fields) manually. Also if the message later changes and a field is added the existing checks would be incorrect.

@tfoote
Copy link
Contributor

tfoote commented Jun 11, 2018

I found this article with a good overview: https://hynek.me/articles/hashes-and-equality/

@dirk-thomas
Copy link
Member Author

Should we remove the __hash__ method based on that?

@tfoote
Copy link
Contributor

tfoote commented Jun 11, 2018

From: https://docs.python.org/3.5/reference/datamodel.html#object.__hash__

If a class defines mutable objects and implements an eq() method, it should not implement hash(), since the implementation of hashable collections requires that a key’s hash value is immutable (if the object’s hash value changes, it will be in the wrong hash bucket).

I think it's right to implement __eq__ but drop the __hash__ implementation.

@dirk-thomas dirk-thomas changed the title generate __eq__ and __hash__ for Python messages generate __eq__ for Python messages Jun 11, 2018
@dirk-thomas
Copy link
Member Author

drop the __hash__ implementation.

Done in 2756c3f.

@dirk-thomas dirk-thomas merged commit 48a69a3 into master Jun 11, 2018
@dirk-thomas dirk-thomas deleted the eq_hash_python_msgs branch June 11, 2018 22:30
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jun 11, 2018
@dhood
Copy link
Member

dhood commented Jun 11, 2018

FWICT ros1 time msg types are hashable: https://github.com/ros/genpy/blob/b244f2f50813ce0385c8673a899bce9e140a35d8/src/genpy/rostime.py#L127..L131 which might cause migration issues to ros2 for some code (unless times in ros2 are immutable? I don't think so). Not much to change, just an observation that came up from the hashable discussion which might be worth adding to the migration guide?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants