Skip to content

vastly improved enum support, serializer returns the value#5

Merged
Sytten merged 3 commits into
rquickjs:mainfrom
miko3k:main
Jun 5, 2026
Merged

vastly improved enum support, serializer returns the value#5
Sytten merged 3 commits into
rquickjs:mainfrom
miko3k:main

Conversation

@miko3k

@miko3k miko3k commented May 3, 2026

Copy link
Copy Markdown
Contributor

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

@Sytten Sytten left a comment

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.

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
Contributor 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.

@Sytten Sytten merged commit ca0e0b0 into rquickjs:main Jun 5, 2026
1 check passed
@Sytten

Sytten commented Jun 5, 2026

Copy link
Copy Markdown
Member

Thanks for your patience

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