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

serde serializer + deserializer for NvList #75

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahrens
Copy link
Contributor

@ahrens ahrens commented Sep 21, 2021

Add a serde serializer that can serialize structs to NvList's.

Add a serde deserializer that can deserialize NvList's into structs.

I'd consider this prototype-quality. I'm looking for feedback on if this would be useful, and how much additional functionality would be needed to get it integrated (or make it useful for folks). Things that are known to be missing:

  • error handling (return Errors rather than todo!() on unimplemented functionality)
  • finer-grained Error variants (not always falling back on Error::Message)
  • deserializer: option to fail on unconsumed pairs?
  • deserializer: handle nvlist_array-type pairs
  • serializer: handle sequences (arrays, vec's, etc) of types other than Byte and NvList
  • serializer: handle tuples
  • serializer: handle enums (variants)
  • automated testing

@ahrens ahrens marked this pull request as draft September 21, 2021 21:28
Add a serde serializer that can serialize structs to NvList's.

Add a serde deserializer that can deserialize NvList's into structs.
@ahrens
Copy link
Contributor Author

ahrens commented Oct 13, 2021

@ahl do you have any thoughts on which of the missing items should be required before integration?

@codyps
Copy link
Owner

codyps commented Oct 16, 2021

I'm generally in favor of having serde support for NvList. Would make working with some NvList content a lot quicker. Might be worth considering hiding behind a feature because serde is a bit heavy (could be default enabled), but that wouldn't block merging this.

@ahrens
Copy link
Contributor Author

ahrens commented Oct 18, 2021

Thanks! I'll look into having serde be a configurable feature, and improving the error handling at least.

@sempervictus
Copy link

@ahrens: does draft status mean this shouldn't be used because its somehow deficient/dangerous, or just that it hasn't been thoroughly used/QA'd?

@ahrens
Copy link
Contributor Author

ahrens commented Mar 16, 2023

@sempervictus We are using this in production (this version to be specific). However, there may be some corner cases where it doesn't work or has confusing results where the nvpair data model doesn't match up with serde's. I would want to document those more thoroughly, and ideally add some unit tests, before this is integrated.

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.

None yet

3 participants