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.

Sign up for GitHub

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

Jump to bottom

feat: Support generics on messages attribute in main contract call #238

Merged
merged 1 commit into from
Oct 19, 2023
Merged

feat: Support generics on messages attribute in main contract call #238

merged 1 commit into from
Oct 19, 2023

Conversation

jawoznia
Copy link
Collaborator

@jawoznia jawoznia commented Oct 9, 2023 •
edited
Loading

close #226

@jawoznia jawoznia requested a review from hashedone October 9, 2023 09:57
@jawoznia jawoznia force-pushed the support_generic_interface_on_contract branch from 8470f7f to 9a0ac19 Compare October 10, 2023 11:27
@jawoznia jawoznia force-pushed the support_generic_messages_on_contract branch from 4e37906 to 04e0edd Compare October 10, 2023 15:25
@codecov
Copy link

codecov bot commented Oct 10, 2023 •
edited
Loading

Codecov Report

Merging #238 (fd40f3d) into feat/generics_support (f046d9e) will decrease coverage by 0.08%.
The diff coverage is 89.28%.

@@                    Coverage Diff                    @@
##           feat/generics_support     #238      +/-   ##
=========================================================
- Coverage                  88.36%   88.29%   -0.08%     
=========================================================
  Files                         25       25              
  Lines                       1444     1478      +34     
=========================================================
+ Hits                        1276     1305      +29     
- Misses                       168      173       +5
Files Coverage Δ
sylvia-derive/src/interfaces.rs 90.65% <100.00%> (+2.14%) ⬆️
sylvia-derive/src/message.rs 87.35% <100.00%> (ø)
sylvia/tests/generics.rs 0.00% <0.00%> (ø)
sylvia-derive/src/parser.rs 83.09% <64.28%> (-1.33%) ⬇️

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

@jawoznia jawoznia force-pushed the support_generic_interface_on_contract branch 4 times, most recently from 6dc3bc6 to 629f3a2 Compare October 18, 2023 10:39
Base automatically changed from support_generic_interface_on_contract to feat/generics_support October 18, 2023 10:57
@jawoznia jawoznia force-pushed the support_generic_messages_on_contract branch from 04e0edd to fd40f3d Compare October 18, 2023 11:07
hashedone
hashedone reviewed Oct 18, 2023
View reviewed changes
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.

Seems mostly good, but I missed some tests. What happens if I try to implement the same interface twice with different generics? I think it should fail due to name collision. Also - can the contract itself be generic at this point? I believe not, right?

One thing I really miss is the tests of multitest utilities generated.

sylvia-derive/src/interfaces.rs Show resolved Hide resolved
sylvia-derive/src/interfaces.rs Show resolved Hide resolved
@jawoznia
Copy link
Collaborator Author

Seems mostly good, but I missed some tests. What happens if I try to implement the same interface twice with different generics? I think it should fail due to name collision. Also - can the contract itself be generic at this point? I believe not, right?

One thing I really miss is the tests of multitest utilities generated.

I tested it with some more interfaces in this PR #239.
Is it cool to deliver it separately?

Yes you cannot implement same interface twice due to name collisions.

hashedone
hashedone approved these changes Oct 19, 2023
View reviewed changes
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.

With your explanations - LGTM

@jawoznia jawoznia merged commit e4cf543 into feat/generics_support Oct 19, 2023
6 checks passed
@jawoznia jawoznia deleted the support_generic_messages_on_contract branch October 19, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashedone hashedone hashedone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jawoznia @hashedone