-
-
Notifications
You must be signed in to change notification settings - Fork 544
fix: Serialize falsey values #906
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
Conversation
src/transform/request.ts
Outdated
@@ -6,7 +6,7 @@ export function transformRequestBodies(requestBodies: Record<string, RequestBody | |||
let output = ""; | |||
|
|||
for (const [name, requestBody] of Object.entries(requestBodies)) { | |||
if (requestBody && requestBody.description) output += ` ${comment(requestBody.description)}`; | |||
if (requestBody && requestBody.description !== undefined) output += ` ${comment(requestBody.description)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Great fix, thank you!
I’m in favor of this approach, but there are some test failures now where empty JSDoc comments are being inserted into the schema. I’d like to avoid this if possible. But when that’s fixed, happy to approve & merge! |
Are you referring to the empty JSDoc comments like this? /** */ They definitely seem kind of weird, but I was going with the perspective "The developer put an empty string in here, for some reason…, so respect that." https://youtu.be/OIVIDmuJZLI?t=40 One would really have to go out of their way in order to introduce these empty comments. Are you saying you'd like me to ignore descriptions and such that would generate such comments, or that you'd like me to fix the tests to match the PR behavior? |
I see that in the spec, for many fields like response-code definitions, that the I agree that these comments are unsightly in this case when there's no mechanism available to avoid them, and I'll see about cleaning up those instances. |
When a prettier config file is specified but can't be found a default prettier config is silently used as a fallback. This change causes an error to be raised when the specified prettier config doesn't exist. Closes openapi-ts#908
cd141ae
to
4e7b1e5
Compare
I've re-worked this PR to only serialize falsey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
@all-contributors please add @duncanbeevers for code, bug, test |
I've put up a pull request to add @duncanbeevers! 🎉 |
* fix: Error on missing prettier config When a prettier config file is specified but can't be found a default prettier config is silently used as a fallback. This change causes an error to be raised when the specified prettier config doesn't exist. Closes openapi-ts#908 * fix: Serialize falsey defaults and examples Closes openapi-ts#890
* docs: add duncanbeevers as a contributor for code, bug, test (#913) * docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> * fix: Serialize falsey values (#906) * fix: Error on missing prettier config When a prettier config file is specified but can't be found a default prettier config is silently used as a fallback. This change causes an error to be raised when the specified prettier config doesn't exist. Closes #908 * fix: Serialize falsey defaults and examples Closes #890 Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Closes #890
I went through and looked for all the spots where an empty string made sense and would be translated across to the TS files.
This includes
example
,description
,title
,format
, anddefault
.I tried to fix this up for both v2 and v3 schemas.