Skip to content

Conversation

@setrofim
Copy link
Contributor

Both these enums have similar definitions and serve similar purposes. To minimize duplication, remove ExtensionValue and replace it with RawValue inside Extensions implementation. Additional functionality of ExtensionValue is added to RawValue, and ExtensionKind is renamed to RawValueKind.

This also resolves issues with serialization of extensions, as ExtensionValue serialzation for maps and sequences was not implemented correctly, unlike in RawValue.

This addresses #32

@setrofim setrofim mentioned this pull request Jan 15, 2025
@setrofim setrofim force-pushed the ext-values branch 3 times, most recently from 8af8c75 to 0855a8d Compare January 15, 2025 15:27
Copy link

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM

Tested this branch with Trustee and it seems good.

Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

LGTM

Both these enums have similar definitions and serve similar purposes. To
minimize duplication, remove ExtensionValue and replace it with RawValue
inside Extensions implementation. Additional functionality of
ExtensionValue is added to RawValue, and ExtensionKind is renamed to
RawValueKind.

This also resolves issues with serialization of extensions, as
ExtensionValue serialzation for maps and sequences was not implemented
correctly, unlike in RawValue.

Signed-off-by: Sergei Trofimov <[email protected]>
Elide some needless lifetimes that are now being reported by clippy.

Signed-off-by: Sergei Trofimov <[email protected]>
To reflect the corresponding license change in the dependencies.

Signed-off-by: Sergei Trofimov <[email protected]>
@setrofim setrofim merged commit 5b1f869 into main Jan 15, 2025
4 checks passed
@setrofim setrofim deleted the ext-values branch January 15, 2025 18:39
@fitzthum fitzthum mentioned this pull request Aug 5, 2025
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.

4 participants