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

Allow functions with return type '-> ()' to be bound #123

Merged
merged 3 commits into from
Jul 6, 2022
Merged

Allow functions with return type '-> ()' to be bound #123

merged 3 commits into from
Jul 6, 2022

Conversation

sagacity
Copy link
Contributor

@sagacity sagacity commented Jul 4, 2022

This deals with one part of #88. I'll handle the T: Serializable generic bound in another pull request.

@sagacity sagacity requested a review from a team as a code owner July 4, 2022 10:06
arendjr
arendjr approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@arendjr arendjr 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 like a good improvement 👍

I do wonder, let's say we face a return type such as Result<(), Error>, wouldn't that still be an issue if () doesn't implement Serializable?

@sagacity
Copy link
Contributor Author

sagacity commented Jul 4, 2022

Fair enough, I don't think cases like Result<(), Error> are being covered now. This PR just addresses a parsing issue in the macro, but we could definitely explore making the solution more generic. I'll see what I can do (and fix the Deno tests, whoops).

@sagacity sagacity marked this pull request as draft July 4, 2022 15:21
@sagacity sagacity marked this pull request as ready for review July 6, 2022 08:11
@sagacity
Copy link
Contributor Author

sagacity commented Jul 6, 2022

@arendjr this seems good to go.

arendjr
arendjr approved these changes Jul 6, 2022
View reviewed changes
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Nice work!

examples/example-protocol/src/assets/rust_plugin_test/expected_import.rs
pub fn import_void_function_empty_result() -> Result<(), u32>;

#[fp_bindgen_support::fp_import_signature]
pub fn import_void_function_empty_return();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is functionally a duplicate of import_void_function() :)

Copy link
Contributor Author

@sagacity sagacity Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, but the protocol specifies a -> () return type here, which gets omitted during codegen. Once that is done, the two functions are indeed identical.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, I overlooked that 👍

@sagacity sagacity merged commit 79795cc into main Jul 6, 2022
@sagacity sagacity deleted the empty-return branch July 6, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arendjr arendjr arendjr 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
@sagacity @arendjr