Add SerializeBits/DeserializeBits derive macros
#84
Conversation
There was a problem hiding this comment.
Thank you!
I will not hold this back a lot, but does internal deserialize code always look this complicated? 🤣
It seems like you can do something better with the deserialize_field_parts inlined and maybe inverting the logic a bit.
You can try, but it would be enough to add tests for arb ints before merging, see comment.
| } | ||
| } | ||
|
|
||
| fn deserialize_field_parts( |
| field_visit_map_match, | ||
| field_visit_map_check, | ||
| mut field_expecting, | ||
| ): (Vec<_>, Vec<_>, Vec<_>, Vec<_>, Vec<_>, Vec<_>, Vec<_>, Vec<_>) = match struct_data.fields { |
tests/serde.rs
Outdated
There was a problem hiding this comment.
These tests are nice, but we have to have one testing an arbitrary integer field ser-de 😄
|
Let me know if those tests look any better or if you have more cases you want me to add. I'll work on cleaning up the logic you pointed out before this gets merged. ime internal deserialize code is usually pretty complicated just because of how serde requires it to be implemented; if there is a better way I'd be happy to do it like that instead. |
|
(running CI to run tests, feel free to clean up still) |
|
I took a stab at cleaning up those ugly tuple types and couldn't come up with anything that didn't look uglier to me :(. That being said I wouldn't be offended if you or someone else took a hacksaw to my stuff. I don't have anything else to offer for this PR. |
There was a problem hiding this comment.
Sorry, I was away again. The nightly test fail is not important rn, but your new tests are not actually running, I didn't realize this earlier. We can just change the workflow file to do cargo test --workspace --features serde.
Again something I could do myself, but I dislike when others fiddle around in my changeset, so I don't do so in others changesets either.
|
We got it :) Thank you again. |
Draft pr for issue #74