Skip to content

Serde Deserialize+Serialize implementation#440

Merged
keepsimple1 merged 9 commits intokeepsimple1:mainfrom
rabbit-time:main
Mar 16, 2026
Merged

Serde Deserialize+Serialize implementation#440
keepsimple1 merged 9 commits intokeepsimple1:mainfrom
rabbit-time:main

Conversation

@rabbit-time
Copy link
Copy Markdown
Contributor

This PR introduces a new feature flag: serde

I've implemented the Serialize and Deserialize traits using derive for ResolvedService and for all the local types which it contains.

Additionally, I've also written a basic test to accompany the changes and added serde_json as a development dependency.

If there is a benefit to providing serde support for types other than ResolvedService, please comment that.

Closes: #438

@rabbit-time
Copy link
Copy Markdown
Contributor Author

CI jobs are currently failing since clippy isn't being run with features enabled.

Line 1286 in ./src/service_info.rs:
#[cfg_attr(feature = "serde", derive(Deserialize, Serialize))]

Copy link
Copy Markdown
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

Looks good! One inline comment related to the CI error. And optional, if you have bandwidth, you can also add a new step in CI to run cargo clippy --tests --all-features -- -D warnings to cover the features (I think).

@rabbit-time
Copy link
Copy Markdown
Contributor Author

Also, one small thing to note. Currently, when there is no scope_id on an IPv6 address, it just puts in blank values.

@keepsimple1 My question to you is, should this be considered intended behavior as it reflects the internal representation with how the struct is implemented in Rust (uses default empty values as opposed to Option<T>) or should it be optimized for size and omit these fields in serialization?

image

@keepsimple1
Copy link
Copy Markdown
Owner

should this be considered intended behavior as it reflects the internal representation with how the struct is implemented in Rust (uses default empty values as opposed to Option) or should it be optimized for size and omit these fields in serialization?

I think we should keep it as is because scope_id is not Option in ScopedIpV6. It's empty here is because it's manually constructed in a test. It would not be empty in a real world service. (Or did I miss something?)

Btw, now ScopedIpV4 also got interface_id, which is the same as scope_id for IPv6. (see merged PR #439 ).

@rabbit-time
Copy link
Copy Markdown
Contributor Author

I think we should keep it as is because scope_id is not Option in ScopedIpV6. It's empty here is because it's manually constructed in a test. It would not be empty in a real world service. (Or did I miss something?)

Btw, now ScopedIpV4 also got interface_id, which is the same as scope_id for IPv6. (see merged PR #439 ).

To my knowledge, an IPv6 address is not always accompanied by a scope ID. I may be wrong.


I will work on finalizing this PR today.

@rabbit-time
Copy link
Copy Markdown
Contributor Author

Alright everything should be ready to go!

If you'd like, take one last look over everything. I moved the serde test into its own submodule so clippy would stop complaining about unused imports.

Copy link
Copy Markdown
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@keepsimple1 keepsimple1 merged commit c2c2f75 into keepsimple1:main Mar 16, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Deserialize/Serialize for mdns-sd types

2 participants