Skip to content

Conversation

@barnjamin
Copy link
Contributor

@barnjamin barnjamin commented Jun 7, 2022

Adding tests for the get_method-by-name methods in the SDKs for Interface and Contract objects. They should return the method with the name if there is only 1 and error if none or >1 are found

Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

Looking good - just wondering what would happen if the method name is not unique and if there is a test case for that.

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

just one comment

@barnjamin barnjamin force-pushed the get-method-by-name branch from f7b3cdd to e2036cb Compare June 8, 2022 18:50
@barnjamin barnjamin force-pushed the get-method-by-name branch from 60e319d to 126f53f Compare June 8, 2022 19:07
Copy link
Contributor

@algochoi algochoi 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 good to me :shipit:

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

LGTM but we can't really merge this until all the SDK's implement. If you want to merge earlier, you can add a special tag to the scenarios such as @abi.contract which will cause the scenarios to be skipped in non-implementing SDK's.

@tzaffi
Copy link
Contributor

tzaffi commented Jun 9, 2022

Another request also before merging - can you add a PR description?

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for making tests

@barnjamin barnjamin requested a review from tzaffi June 13, 2022 16:27
@barnjamin barnjamin merged commit 33cc77f into master Jun 13, 2022
@barnjamin barnjamin deleted the get-method-by-name branch June 13, 2022 16:35
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.

6 participants