Skip to content

Conversation

@mashabow
Copy link
Contributor

@mashabow mashabow commented Oct 31, 2023

Types of changes

  • Features
    • resolves #
  • Bug fixes
    • resolves #
  • Maintenance
  • Documentation

Changes

I found that when we have an empty object for the response body, it is outputted as void type instead of {} type. So, I fixed it.

Since I teaked schema2value, it might affect other parts beyond just the response body. However, based on the existing tests, everything seems okay.

レスポンスボディが空オブジェクトのとき、{} 型ではなく void 型で出力される場合があるようなので、修正してみました。

schema2value をいじったので、レスポンスボディだけでなく他の部分にも影響すると思いますが、既存のテストを見るかぎりでは悪影響はなさそうです。

Additional context

Community note

Please upvote with reacting as 👍 to express your agreement.

Comment on lines 18 to 23
resBody: {
limit: number
offset: number
data: {
}[]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

この部分のスキーマは以下のようになっていました。

"schema": {
"required": ["data", "limit", "offset"],
"properties": {
"limit": { "type": "number" },
"offset": { "type": "number" },
"data": { "type": "array", "items": { "type": "object" } }
}
}

ここは「レスポンスボディが空オブジェクトだった場合」でなく「レスポンスボディの一部分が空オブジェクトだった場合」ですが、修正後の {}[] の方が正しく型を表しています。

isArray = true;
value = schema2value(schema.items);
} else if (schema.properties || schema.additionalProperties) {
} else if (schema.type === 'object' || schema.properties || 'additionalProperties' in schema) {
Copy link

@mikana0918 mikana0918 Jan 6, 2024

Choose a reason for hiding this comment

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

Suggested change
} else if (schema.type === 'object' || schema.properties || 'additionalProperties' in schema) {
} else if (schema.type === 'object' || schema.properties || schema.additionalProperties) {

@mashabow PRの確認遅くなっておりすみません。こちら他の箇所と同様に schema.additionalPropertiesとするのはいかがでしょうか? (それに関して問題等ありそうでしょうか。)もし特段の理由がなければ、ドットでのアクセスのほうが周りのコードと乖離がなく、読む際に迷いもなくなるので、いいかな?と思いました。

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