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

fix: remove invalid assertion #3018

Merged
merged 1 commit into from Apr 21, 2021
Merged

fix: remove invalid assertion #3018

merged 1 commit into from Apr 21, 2021

Conversation

RafaelGSS
Copy link
Member

Ref #3017

I removed the assertion since it does not seem useful, FastifyError is an interface and in the test are being called as a function.
@Ethan-Arrowood I'm asking review for you since you've implemented it a long time ago.

Checklist

@RafaelGSS RafaelGSS requested review from Ethan-Arrowood and a team April 21, 2021 01:48
@github-actions github-actions bot added test Issue or pr related to our testing infrastructure. typescript TypeScript related labels Apr 21, 2021
@RafaelGSS RafaelGSS linked an issue Apr 21, 2021 that may be closed by this pull request
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

I think this error is related to the changes in fastify/fastify-error@062c258.

I am +1 on removing this test case as you said FastifyError itself is only the interface and can not be construable or called directly.

Can we change the test to expectType<ValidationResult[] | undefined>(FastifyError.validation) to see if the error persist?

@RafaelGSS
Copy link
Member Author

I think this error is related to the changes in fastify/fastify-error@062c258.

Agree.

Can we change the test to expectType<ValidationResult[] | undefined>(FastifyError.validation) to see if the error persist?

Not sure if it's worth it since we are checking the interface types it doesn't sound redundant for you?

@climba03003
Copy link
Member

I think this error is related to the changes in fastify/fastify-error@062c258.

Agree.

Can we change the test to expectType<ValidationResult[] | undefined>(FastifyError.validation) to see if the error persist?

Not sure if it's worth it since we are checking the interface types it doesn't sound redundant for you?

Yes, it is. Let's wait for @Ethan-Arrowood response to see if this checking have any actual meanings.
I'm fine for both remove and modify the checking.

@mcollina
Copy link
Member

I'm sorry but the build is broken. I need this fixed asap.

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

I need to ship a release and this is a blocker.

@Ethan-Arrowood PTAL

@mcollina mcollina merged commit aac2388 into fastify:main Apr 21, 2021
@RafaelGSS RafaelGSS mentioned this pull request Apr 22, 2021
4 tasks
@github-actions
Copy link

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 Apr 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test Issue or pr related to our testing infrastructure. typescript TypeScript related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken typescript test
3 participants