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

Add missing type #2400

Merged
merged 3 commits into from Jul 14, 2020
Merged

Add missing type #2400

merged 3 commits into from Jul 14, 2020

Conversation

chasingSublimity
Copy link
Contributor

@chasingSublimity chasingSublimity commented Jul 11, 2020 •
edited

Fixes #2358

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

tests-checker[bot]
tests-checker bot suggested changes Jul 11, 2020
View reviewed changes
Copy link

@tests-checker tests-checker bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for contributing!
It appears that you have changed the framework code, but the tests that verify your change are missing. Could you please add them?

mcollina
mcollina requested changes Jul 11, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test?

@chasingSublimity
Copy link
Contributor Author

Hey @Ethan-Arrowood! As mentioned in #2358, I could use a bit of guidance with the tests here. I imagine that a test needs to be added to instance.test-d.ts. I'm not quite sure what that test should entail, however. Maybe adding a check that verifies setReplySerializer is a valid instance method?

Thanks!

@Ethan-Arrowood
Copy link
Member

@chasingSublimity you can use the setErrorHandler types as a template. For your test. And yes that is the correct file.

@chasingSublimity
Copy link
Contributor Author

@mcollina @Ethan-Arrowood I added a few type tests. Let me know if any further changes are needed! 🚀

@chasingSublimity chasingSublimity changed the title [WIP] Add missing type Add missing type Jul 13, 2020
Ethan-Arrowood
Ethan-Arrowood approved these changes Jul 13, 2020
View reviewed changes
Copy link
Member

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

LGTM

mcollina
mcollina approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 2495ba1 into fastify:master Jul 14, 2020
@chasingSublimity
Copy link
Contributor Author

thanks for the help!

@chasingSublimity chasingSublimity deleted the Add-missing-type branch July 14, 2020 12:40
@github-actions
Copy link

github-actions bot commented Feb 9, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tests-checker tests-checker[bot] tests-checker requested changes

@mcollina mcollina mcollina approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood 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.

Error: Property 'setReplySerializer' does not exist on type 'FastifyInstance'
3 participants
@chasingSublimity @Ethan-Arrowood @mcollina