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

Add native function declarations for all relevant Test contract functions #2926

Conversation

m-Peter
Copy link
Contributor

@m-Peter m-Peter commented Nov 6, 2023

Closes #2822

Description

With the introduction of #2821, we can now declare native functions inside the Test contract, and remove the workaround that used CompositeType.ResolveMembers().

To do that, we also had to enable default arguments for native functions, since Test.assert and Test.fail both have an optional message argument.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@m-Peter m-Peter changed the base branch from feature/destructor-removal to feature/remove-destructors November 6, 2023 15:18
@SupunS SupunS self-assigned this Nov 6, 2023
@m-Peter m-Peter changed the base branch from feature/remove-destructors to feature/stable-cadence November 8, 2023 06:57
@m-Peter m-Peter force-pushed the add-test-contract-native-functions branch from 9ebf7d6 to e2a302c Compare November 8, 2023 07:04
@SupunS
Copy link
Member

SupunS commented Nov 8, 2023

Does this replace #2823? I see that the #2823 is on top of master. Do we need both, or does having this change on the stable-cadence branch make it easier to do the change?

@m-Peter
Copy link
Contributor Author

m-Peter commented Nov 8, 2023

Oh, right, I forgot to close #2823 🤦
This actually uses some changes for default arguments, which were introduced from default resource destroyed events.
I have added support for parsing default arguments in native functions, however I am not sure on how to proceed with the checking part.

@dsainati1 dsainati1 deleted the branch onflow:feature/stable-cadence December 11, 2023 14:57
@dsainati1 dsainati1 closed this Dec 11, 2023
@SupunS
Copy link
Member

SupunS commented Dec 11, 2023

Not sure why this got closed. This should now be opened against the master (#2971)

@m-Peter
Copy link
Contributor Author

m-Peter commented Dec 11, 2023

@SupunS Because the onflow:feature/stable-cadence was merged on master, and then deleted. I will rebase on top of the new changes that got added in between, and I'll re-open against the master branch.

@SupunS
Copy link
Member

SupunS commented Dec 11, 2023

Yeah, usually it should get automatically rebased. But thank you for reopening 🙏

@m-Peter
Copy link
Contributor Author

m-Peter commented Dec 11, 2023

There are some conflicts that need to be resolved by hand, that's why it didn't get automatically rebased probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants