Skip to content

vastly improved enum support, serializer returns the value#5

Open
miko3k wants to merge 3 commits into
rquickjs:mainfrom
miko3k:main
Open

vastly improved enum support, serializer returns the value#5
miko3k wants to merge 3 commits into
rquickjs:mainfrom
miko3k:main

Conversation

@miko3k
Copy link
Copy Markdown

@miko3k miko3k commented May 3, 2026

Hi,

What a cool crate you guys made.

My idea was to add proper enum serialization and deserialization.

Deserialization was fairly straightforward.

Serialization turned out to be a bit more difficult, as I couldn’t quite wrap my head around state management in the Serializer. This commit reworks the approach and reimplements the logic in a tree-builder style. Note that this also introduces an API change, as the Serializer now returns the resulting value, just like the Deserializer does.

Unit tests are included.

Assisted by: ChatGPT was used to answer questions; no vibe coding took place.

Cheers.

@miko3k miko3k requested a review from Sytten as a code owner May 3, 2026 18:07
Copy link
Copy Markdown
Member

@Sytten Sytten left a comment

Choose a reason for hiding this comment

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

I reviewed the deserializer and it looks good, just one small comment to change.
I still need to review the serializer, sorry this is taking longer that I would wish.

Comment thread src/de.rs Outdated
@@ -592,60 +606,69 @@ fn get_index<'a>(obj: &Object<'a>, idx: usize) -> rquickjs::Result<Value<'a>> {
}

/// A helper struct for deserializing enums containing unit variants.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wrong comment now

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yep. I adjsuted the comment. Like I understand my changes to serializer are significant, but trying to keep current previous architecture while supporting enum full would be very challenging for the whole state managment.

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.

2 participants