Skip to content

Use Magnus macros to implement TypedData trait#159

Merged
jbourassa merged 1 commit intobytecodealliance:mainfrom
matsadler:derive-typed-data
Mar 29, 2023
Merged

Use Magnus macros to implement TypedData trait#159
jbourassa merged 1 commit intobytecodealliance:mainfrom
matsadler:derive-typed-data

Conversation

@matsadler
Copy link
Copy Markdown
Contributor

This library is a great stress test for Magnus' API, so I've been trying out some Magnus API changes to see how they fair in use.

While I was doing that I noticed there's a lot of implementations of TypedData that are near identical to what would be derived.

Magnus doesn't usually allow deriving TypedData for types with generics as the derived implementation isn't guaranteed to be correct, and I don't want people trying it, find it compiles, and then hit nasty segfaults.

However, it seems mean to make you write out exactly what the macro would produce, so I added an unsafe_generics attribute to bypass the error, and I've back ported it to the current release branch of the magnus-macros crate.

The changes here remove the TypedData implementations in favour of the macros with the unsafe_generics attribute set.

@ianks
Copy link
Copy Markdown
Collaborator

ianks commented Mar 29, 2023

This looks great, thank you @matsadler. I think a cargo clippy --fix should make CI pass for ya.

Magnus doesn't usually allow deriving TypedData for types with
generics as the derived implementation isn't guaranteed to be
correct.

The implementations here are near identical to what would be
derived, so use the new 'unsafe_generics' attribute to bypass
the usual error.
Copy link
Copy Markdown
Collaborator

@jbourassa jbourassa left a comment

Choose a reason for hiding this comment

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

This looks great, thank you @matsadler.

+1, fantastic!

@jbourassa jbourassa merged commit e81f001 into bytecodealliance:main Mar 29, 2023
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.

3 participants