Closed kuniyuki-f closed 1 year ago
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Comments | Updated (UTC) |
---|---|---|---|---|
timelogger-web | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Jul 20, 2023 6:45am |
なぜなら、本来値が入るべき response を返すエンドポイントに request する場合(例: 今回実装したリクエスト処理)、主にバリデーション関連の実装がやりやすくなるかなーと思ったからです。
ちなみにこれに関しても自分は賛成です!
基本的にオプショナルな値が多いと都度存在チェック等が必要で扱いにくいobjectになってしまうからです!
なぜなら、本来値が入るべき response を返すエンドポイントに request する場合(例: 今回実装したリクエスト処理)、主にバリデーション関連の実装がやりやすくなるかなーと思ったからです。
ちなみにこれに関しても自分は賛成です!
基本的にオプショナルな値が多いと都度存在チェック等が必要で扱いにくいobjectになってしまうからです!
ありがとうございます! OpenAPIの定義変更イシューを作成しました。
※ 本 PR では Task のプロパティ必須化の対応はしません💡
@keitakn
昨日頂いたコメントの修正が最後でそれ以外はLGTMもらっていることと、CIのテストクリアしてることからマージしてしまいますね!
今回もレビューありがとうございました!
issueURL
59
この PR で対応する範囲 / この PR で対応しない範囲
Storybook の URL、 スクリーンショット
API リクエストの関する処理の実装なので今回はなし
変更点概要
主に /tasks/recording へのリクエスト処理に関する実装を行いました。
レビュアーに重点的にチェックして欲しい点
コード内にコメントします。 また、補足情報についてもご確認いただきたいです🙏
補足情報
下記について、以前 #88 の対応で keita さんと議論になりました。 当時と少し考えが変わったので、改めてレビュワーの方のご意見お伺いしたいです🙇
現在
Task
の型定義をoptional
にしています (taskCategoryId
以外 )。 理由は以下のとおりです。Task
のスキーマを openApi の定義に合わせるためtaskCategoryId
以外不要だったためしかし今回の実装中に
Task
の取り回しを考慮すると他のキーもrequired
にした方が良いかもしれないと感じました。なぜなら、本来値が入るべき response を返すエンドポイントに request する場合(例: 今回実装したリクエスト処理)、主にバリデーション関連の実装がやりやすくなるかなーと思ったからです。
(自分は
stopTask
やcreateTask
を実装しているときは上記を考慮できていなかったです🙏)上記について、いかがでしょうか。 どうぞレビューのほどよろしくお願い致します!
(補足情報)
Task のスキーマ
openApi の schema.ts