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

chore(runtime): correct return types for completeCallback and processRequest #4143

Merged
merged 3 commits into from
Jun 13, 2023

Conversation

TimothyJones
Copy link
Contributor

The return type for completeCallback was incorrectly listed as void, which confused me for a bit. This PR corrects it.

Changes here:

  • I've changed the return type of completeCallback to api.CompleteRequest['result'] (was void) as this is correct. Technically, this resolves to any - but I thought api.CompleteRequest['result'] was more explicit about what is actually happening.
  • I templated the callback argument to processRequest so that the return type is correct there, too.
  • I've given both handleCallback and explicit return types. This is personal preference - let me know if you'd prefer the functions left with implicit return types.

As an aside, I was surprised the old version compiled, but it turns out you're allowed to return an any from a function marked as returning void. Makes sense as you can assign any to anything, but it was a bit surprising.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mergify
Copy link
Contributor

mergify bot commented Jun 12, 2023

The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged.
[Conventional Commits]: https://www.conventionalcommits.org

@TimothyJones TimothyJones changed the title Correct return type chore(runtime): correct return types for completeCallback and processRequest Jun 12, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2023

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Jun 13, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2023

Merging (with squash)...

@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2023

Merging (with squash)...

@mergify mergify bot merged commit 70c1462 into aws:main Jun 13, 2023
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Jun 13, 2023
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.

2 participants