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

Split compose_call into two functions #706

Merged
merged 7 commits into from
Feb 15, 2024
Merged

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Feb 5, 2024

closes #598

@haerdib haerdib self-assigned this Feb 5, 2024
@haerdib haerdib force-pushed the bh/598-split-compose-call branch from e0e3e85 to 7c587a7 Compare February 5, 2024 14:46
@haerdib haerdib marked this pull request as ready for review February 5, 2024 14:50
@haerdib haerdib requested a review from masapr February 5, 2024 14:50
@haerdib haerdib added F7-enhancement Enhances an already existing functionality E1-breaksnothing labels Feb 5, 2024
@haerdib haerdib changed the title Split compose_call Split compose_call into two functions Feb 5, 2024
Copy link
Collaborator

@masapr masapr left a comment

Choose a reason for hiding this comment

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

In the issue you say "in case multiple calls for one pallet are created, the current code is forced to get the PalletMetada each time for each call. That's not necessary."

I don't see, how you can create this scenario with the new macros. Would you call
let pallet_metadata = $node_metadata.pallet_by_name($pallet).unwrap().to_owned();
directly and then use the second macro to compose the calls?

Generally, I would propose to write a simple test, with the scenario described in #598. If it can be done nicely with the new code, then I'm fine with it.

@haerdib
Copy link
Contributor Author

haerdib commented Feb 13, 2024

Added an unit test in 688068a. Is that what you imagined?

@haerdib haerdib requested a review from masapr February 14, 2024 09:35
@masapr
Copy link
Collaborator

masapr commented Feb 15, 2024

Added an unit test in 688068a. Is that what you imagined?

Yes. But I would also add some assertion on _call_one and _call_two.
... sorry for being so picky here. I know our code-base is not really written with TDD. But if we specifically change something for a specific purpose, why not check, that what we aim for, actually works.

@haerdib
Copy link
Contributor Author

haerdib commented Feb 15, 2024

Cool! You're right - I fixed it in 5de32cf and moved it to the correct crate as well.

Copy link
Collaborator

@masapr masapr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the test :-)

@haerdib haerdib merged commit 541cd54 into master Feb 15, 2024
54 checks passed
@haerdib haerdib deleted the bh/598-split-compose-call branch February 15, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E1-breaksnothing F7-enhancement Enhances an already existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split compose_call into two functions
2 participants