Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't prepend marker for deserialization of root struct #48

Merged
merged 4 commits into from
Oct 3, 2023

Conversation

Boog900
Copy link
Member

@Boog900 Boog900 commented Jun 27, 2023

I didn't like the old method, this one is better

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we'll need to bump the MSRV in CI. Can you add a changelog entry for that please?

src/de.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title Change how embedded structs are handled Don't prepend marker for deserialization of root struct Jun 27, 2023
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jun 27, 2023

I didn't like the old method, this one is better

To be honest, I am not sure I fully agree. I see it as a short-coming of the data format that the root struct does not have a marker. If you think of JSON for example, the root object is also always correctly identified. Hence, I find it quite easy to understand that we "fix" this short-coming by prepending the marker.

I am not opposed to changing it, just wondering why you think this approach is better.

@Boog900
Copy link
Member Author

Boog900 commented Jun 28, 2023

I agree this is a short coming of the data format but the old method felt too much like a hack for my liking. Plus doing what it was doing before causes the data to be cloned before de-serializing which will reduce performance, there probably is a way to do it without cloning but we may as well just do this.

Boog900 and others added 2 commits October 3, 2023 20:53
Co-authored-by: Thomas Eizinger <[email protected]>
@Boog900 Boog900 merged commit a91cbaa into monero-rs:main Oct 3, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants