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

chore: simplify discriminants handling in schema derives #241

Merged
merged 2 commits into from
Oct 25, 2023

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Oct 6, 2023

There’s no need for a separate discriminant variable and assignment
statement in the derived code. The value of the discriminant can be
included directly in the entry for the variant.

@mina86 mina86 requested review from frol and dj8yfo as code owners October 6, 2023 12:36
There’s no need for a separate discriminant variable and assignment
statement in the derived code.  The value of the discriminant can be
included directly in the entry for the variant.
@mina86
Copy link
Contributor Author

mina86 commented Oct 23, 2023

cargo-doc failure is not of my doing.

@dj8yfo dj8yfo changed the title chore!: simplify discriminants handling in schema derives chore: simplify discriminants handling in schema derives Oct 24, 2023
@@ -139,21 +125,21 @@ fn process_variant(
inner_struct_definition(variant, cratename, &full_variant_ident, enum_generics);
let (_ig, inner_struct_ty_generics, _wc) = inner_struct_generics.split_for_impl();

let add_definitions_recursively_call = quote! {
<#full_variant_ident #inner_struct_ty_generics as #cratename::BorshSchema>::add_definitions_recursively(definitions);
let variant_type = quote! {
Copy link
Collaborator

@dj8yfo dj8yfo Oct 24, 2023

Choose a reason for hiding this comment

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

a better name for variable might be fully_qualified_variant_type

let definition = borsh::schema::Definition::Enum {
tag_width: 1,
variants: borsh::__private::maybestd::vec![
(discriminant_0 as i64, "A".to_string(), < XA as borsh::BorshSchema >
Copy link
Collaborator

Choose a reason for hiding this comment

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

the pr would be simpler to review if it didn't contain to_string -> into changes, which are a little bit off from its subject

Copy link
Collaborator

@dj8yfo dj8yfo left a comment

Choose a reason for hiding this comment

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

Macro derive code was considerably simplified, macro expansion became a little bit less readable.
Despite the pr being originally marked as chore!, i don't see how it can cause a breaking change,
and it appears that it constitutes a patch version change, if released all by itself.

Approving it with accuracy up to a passing ci build.

@dj8yfo dj8yfo merged commit 71a9d4d into near:master Oct 25, 2023
7 checks passed
@frol frol mentioned this pull request Oct 25, 2023
@mina86 mina86 deleted the a branch October 25, 2023 17:22
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