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

adds endpoint-specific transforms #752

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

thorhj
Copy link
Contributor

@thorhj thorhj commented Aug 25, 2023

Addressing #749

Checklist

@thorhj
Copy link
Contributor Author

thorhj commented Aug 25, 2023

Two considerations for the reviewer:

  1. The property for the transform function is called swaggerTransform but supports both OpenAPI and Swagger specs of course. Don't know if that is confusion and a better name should be added (or two separate properties).
  2. The implementation allows the swaggerTransform to be given the value null which has its own specific meaning. Maybe this should be the string literal "disable" or something else to be more clear.

@thorhj thorhj force-pushed the endpoint-specific-transforms branch from 7ea0a17 to bde7dbf Compare August 25, 2023 07:46
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

missing typing tests.

@thorhj
Copy link
Contributor Author

thorhj commented Aug 25, 2023

missing typing tests.

Added some tests in types.test.ts. Let me know if they should be written differently, haven't made such tests before (I'm usually working in Typescript directly 😉)

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

@Uzlopak Uzlopak mentioned this pull request Aug 30, 2023
2 tasks
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

I reviewed it now.

swggerTransform null is quiet unintuitive. I would personally prefer false for deactivating.

index.d.ts Show resolved Hide resolved
Use "false" instead of "null" to disable the global schema transform.
@thorhj
Copy link
Contributor Author

thorhj commented Aug 31, 2023

I reviewed it now.

swggerTransform null is quiet unintuitive. I would personally prefer false for deactivating.

I agree, changed it to false instead of null.

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Uzlopak Uzlopak merged commit a58bcbe into fastify:master Aug 31, 2023
15 checks passed
@fredrikj31
Copy link

When is this set to be released? 🤔

@mcollina
Copy link
Member

mcollina commented Sep 5, 2023

done now

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.

None yet

4 participants