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

Querier generation #122

Merged
merged 16 commits into from
May 24, 2023
Merged

Querier generation #122

merged 16 commits into from
May 24, 2023

Conversation

jawoznia
Copy link
Collaborator

PR2 of #28

@jawoznia jawoznia requested a review from hashedone May 18, 2023 10:36
@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #122 (5e8a29d) into main (a4836ff) will increase coverage by 0.59%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
+ Coverage   79.37%   79.96%   +0.59%     
==========================================
  Files          30       32       +2     
  Lines        1624     1692      +68     
==========================================
+ Hits         1289     1353      +64     
- Misses        335      339       +4     
Impacted Files Coverage Δ
sylvia-derive/src/lib.rs 75.00% <ø> (ø)
sylvia/tests/query_returns.rs 0.00% <ø> (ø)
sylvia/tests/remote.rs 0.00% <ø> (ø)
sylvia-derive/src/variant_descs.rs 70.58% <70.58%> (ø)
sylvia-derive/src/message.rs 75.80% <76.92%> (+2.95%) ⬆️
sylvia/tests/querier.rs 95.23% <95.23%> (ø)
sylvia-derive/src/input.rs 86.44% <100.00%> (+0.47%) ⬆️
sylvia-derive/src/remote.rs 81.25% <100.00%> (-2.09%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

One thing is missing here. In generated MT utilities, we add functions on Queries. I would say, that it would be beneficial to use Queriers in them instead of reusing already generated utilities (to reduce maintenance cost).

I like what is getting generated and how is used, but I don't like the implementation structure.

sylvia-derive/src/message.rs Outdated Show resolved Hide resolved
sylvia-derive/src/message.rs Outdated Show resolved Hide resolved
sylvia-derive/src/querier.rs Outdated Show resolved Hide resolved
sylvia-derive/src/querier.rs Outdated Show resolved Hide resolved
sylvia-derive/src/querier.rs Outdated Show resolved Hide resolved
sylvia/tests/querier.rs Outdated Show resolved Hide resolved
sylvia/tests/querier.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

Far better than the previous iteration, with some additional structure-wise comments.

sylvia-derive/src/message.rs Show resolved Hide resolved
sylvia-derive/src/message.rs Outdated Show resolved Hide resolved
sylvia-derive/src/utils.rs Outdated Show resolved Hide resolved
sylvia/tests/querier.rs Outdated Show resolved Hide resolved
tarpaulin-report.html Outdated Show resolved Hide resolved
}
}

pub struct MsgVariants<'a>(Vec<MsgVariant<'a>>);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if this is a good approach to generate Querier directly from MsgVariants.

Later (in #28 (comment)) when we will implement From on the BoundQuerier we will need to pass the interfaces to emit_querier or source and parse it there inside. It doesn't fit in new so this would be a double pass of source inside the MsgVariants.

We could introduce a new struct, f.e. ParsedInput or however we would call it that would hold all the attributes, messages, error type and so on. This would reduce the preparsing which we currently do in multiple places: multitest, EnumMessage, ContractEnumMessage.
This way we could use this struct (in some separate PR) in other Structs generating code and extract all the commons to be generated by the ParsedInput to avoid duplications and reduce maintanence cost.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing is - whatever is generated able from msg variants itself should be generated from here so we do not expose fields. What you are mentioning is a matter of the responsibility split.

When you need to emit From implementation on BoundQuerier, you can do it in another, maybe a new type, as it does not need access to message variants (it needs only the modules where "forms" are implemented).

Same thing when you implement traits on queries - maybe you will need some additional data, but only for the impl block header, not for functions - you will be able to emit functions implementation from MsgVariants, which would be passed (by ref) to some additional structure, and then the this additional structure will generate inner implementation calling some Msgvariants emit-family function, and then it would add some context (like impl block itself).

Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

LGTM.

Some minor comments, but please apply them.

sylvia-derive/src/as_variant_descs.rs Outdated Show resolved Hide resolved
sylvia-derive/src/as_variant_descs.rs Outdated Show resolved Hide resolved
sylvia-derive/src/as_variant_descs.rs Outdated Show resolved Hide resolved
sylvia-derive/src/as_variant_descs.rs Outdated Show resolved Hide resolved
sylvia-derive/src/as_variant_descs.rs Outdated Show resolved Hide resolved
}
}

pub struct MsgVariants<'a>(Vec<MsgVariant<'a>>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing is - whatever is generated able from msg variants itself should be generated from here so we do not expose fields. What you are mentioning is a matter of the responsibility split.

When you need to emit From implementation on BoundQuerier, you can do it in another, maybe a new type, as it does not need access to message variants (it needs only the modules where "forms" are implemented).

Same thing when you implement traits on queries - maybe you will need some additional data, but only for the impl block header, not for functions - you will be able to emit functions implementation from MsgVariants, which would be passed (by ref) to some additional structure, and then the this additional structure will generate inner implementation calling some Msgvariants emit-family function, and then it would add some context (like impl block itself).

sylvia/tests/querier.rs Show resolved Hide resolved
@jawoznia jawoznia merged commit 46cc1c2 into main May 24, 2023
6 checks passed
@jawoznia jawoznia deleted the generate_querier branch May 24, 2023 09:10
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.

None yet

2 participants