Serialization: Host serialization cleanup #328
Conversation
|
Found some issues. Working on fixing them. |
Glutexo
force-pushed
the
host_serialization_cleanup
branch
from
be58344
to
4c08923
Compare
|
Forgot about fixing canonical facts, so it is deserialized as trusted. Fixed. |
Glutexo
force-pushed
the
host_serialization_cleanup
branch
from
4c08923
to
6cff9a8
Compare
|
Rebased on the current state of split_inventory_service. The pull request is now mergeable again and the tests are 🍏. I was thinking a bit about the internal Python representation of the canonical facts. It would make sense for them to be a namedtuple. I wanted to start with having a dict strictly with None values. That doesn’t work though, because the internal object is currently used as a database model and we need the canonical facts to be stored without the empty values. Otherwise the JSONB comparison to search by canonical facts wouldn’t work. I’d like to see this transition when splitting the database model from the internal objects (#309) split, but that’s not a task for today. |
|
May I ask for a review of this, please? Thanks! |
Glutexo
force-pushed
the
host_serialization_cleanup
branch
from
6cff9a8
to
06507fb
Compare
|
Rebased. |
Glutexo
force-pushed
the
host_serialization_cleanup
branch
from
06507fb
to
1e01204
Compare
|
Rebased to resolve conflicts, but the rebase went without one. Strange. |
Glutexo
force-pushed
the
host_serialization_cleanup
branch
from
1e01204
to
8335d34
Compare
|
Rebased to fix formatting conflicts. |
Glutexo
force-pushed
the
host_serialization_cleanup
branch
from
8335d34
to
41904dc
Compare
|
Rebased to resolve conflicts. Also amended the commits so they don't introduce new rule violations. Removed forgotten debugging-related code. |
Reviewed the style of the (de)serialization methods. Removed redundant or rather cryptic instructions. Leveraged comprehensions, dried a few bits of code. Made the code more concise and expressive. Unified names a bit.
Change the (de)serialization logic, so the serialization expects that the object is valid. All default values and validation happens now upon deserialization. That concentrates the logic into a single place.
Glutexo
force-pushed
the
host_serialization_cleanup
branch
from
41904dc
to
5c099fe
Compare
|
Rebased to resolve conflicts. Mergeable again. I’ll split it into two pull requests, to it’s easier to review. If anybody gets to review before that, feel free to merge. |
Glutexo
changed the title
Did a little cleanup of the (de)serialization methods. This consists of two changes, which I didn’t want to break up as they would be uncomfortably little:
Both should make the code more durable and understandable. Although there is a change in the internal logic of the serialization, there are no changes for the code using the methods. No changes in the test nor in the overall behavior.
There are no unit tests in this pull request as the original methods were already untested. There is a separate PR (#325) with the unit tests. Some of them would have to be slightly changed to be green after merging this. Refer my rebased branch host_serialization_cleanup_with_tests to see the changes.