-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
refactor(backend): core/activitypub/models
#11067
refactor(backend): core/activitypub/models
#11067
Conversation
これまでは`getApId()`の方でエラーがスローされていた。
- `toArray()`を使うように - よくわからない条件式を整理 - `as`をなくすために`promiseLimit()`でジェネリクスを使うように
`res`が`null`でないことは確認されているようだったので`null`とのunionはなくした
エラーメッセージを考える
Codecov Report
@@ Coverage Diff @@
## develop #11067 +/- ##
===========================================
- Coverage 77.81% 77.40% -0.42%
===========================================
Files 171 910 +739
Lines 21487 91760 +70273
Branches 498 7548 +7050
===========================================
+ Hits 16721 71029 +54308
- Misses 4766 20731 +15965
|
backendの ただ、 |
テストに通っていない原因として |
テストにも通るようになったのでdraftを外します。よろしくお願いします。 |
core/activitypub
core/activitypub/models
packages/backend/src/core/activitypub/models/ApPersonService.ts
Outdated
Show resolved
Hide resolved
今にして思えばこのPRはリファクタリングの域から外れるようなことをしてしまっている上に、差分が多すぎて確認もしづらいため、approveしていただいた手前申し訳ないですが一旦なかったことにさせてもらいたいです。後々このPRが参照されるようなことがあったとき、理解する負担を払わせることを考えるとよくない気がしまして。 細かくわけた別のPRとして送り直したいのですが、構いませんか? |
もうマージしちゃっていいのでは (同じ変更を細かく出されてもそれはそれで困るし自鯖にマージしてしまったのでそれをrevertするのが面倒) |
これはたしかにその通りで、私も言ったもののどうわければいいか悩んでいたところです……。 ただ、
の2つがあります。ご指摘を受けた 662d2b1 は、この2つの変更の流れでしてしまったものなのです。 私がきちんと読んでいれば防げたものですので本当に申し訳ないです……。 |
問題のある2つのコミットをrevertしました。conflictさえ解消したらマージできるはずです。 |
conflict解消しました。 |
👍 |
* cleanup(`ApImageService.ts`) * refactor(`ApImageService.ts`) * cleanup(`check-https.ts`) * cleanup(`ApMentionService.ts`) * refactor(`ApMentionService.ts`) * cleanup(`ApNoteService.ts`): unneeded `eslint-disable-next-line` * cleanup(`ApNoteService.ts`) * WIP(`ApImageService.ts`): `image.url`を`getApHrefNullable()`に通すかどうか悩んでいる * refactor(`ApNoteService.ts`): function return type * cleanup(`ApNoteService.ts`): deadcode * refactor(`ApNoteService.ts`): `eslint-disable-next-line` * refactor(`ApNoteService.ts`): non-null assertion これまでは`getApId()`の方でエラーがスローされていた。 * cleanup(`ApNoteService.ts`): unneeded await * refactor(`ApNoteService.ts`): note.attachment - `toArray()`を使うように - よくわからない条件式を整理 - `as`をなくすために`promiseLimit()`でジェネリクスを使うように * cleanup(`ApNoteService.ts`) * refactor(`ApNoteService.ts`): よりよい型定義 `res`が`null`でないことは確認されているようだったので`null`とのunionはなくした * refactor(`ApNoteService.ts`): 不要な条件を削除 * cleanup(`ApNoteService.ts`) * cleanup(`ApNoteService.ts`): 重要でない`as`を削除 * refactor(`ApNoteService.ts`): `eslint-disable-next-line` * cleanup(`ApNoteService.ts`): deadcode * cleanup(`ApNoteService.ts`): unneeded non-null assertion * refactor(`ApNoteService.ts`): 不要な条件を削除 * WIP(`ApNoteService.ts`): `as`をなくす エラーメッセージを考える * cleanup(`ApNoteService.ts`): 不要な`as`を削除 * cleanup(`ApPersonService.ts`): `no-unused-vars` * cleanup(`ApPersonService.ts`): deadcode * refactor(`ApPersonService.ts`): function return type * cleanup(`ApPersonService.ts`): deadcode * cleanup(`ApPersonService.ts`): deadcode * WIP(`ApPersonService.ts`): `as`を調整 `null`でないか確認する処理が続いていたので型アサーションは`null`とのunionにした。 より本質的な改善の余地があるように感じるのでひとまずWIPとしてコミット。 * refactor(`ApPersonService.ts`): `eslint-disable-next-line` * WIP(`ApPersonService.ts`): `as any`をなくした エラーをスローするようにせざるを得なかったのでエラーメッセージを考える必要がある。 * WIP(`ApNoteService.ts`): non-null assertion non-nullアサーションを減らすために事前に存在確認をするようにした。 エラーをスローするようにしたのでメッセージを考えなければならない。 * refactor(`ApNoteService.ts`): non-null assertion -> optional chaining * refactor(`ApPersonService.ts`): `eslint-disable-next-line` * refactor(`ApPersonService.ts`): `eslint-disable-next-line` * refactor(`ApPersonService.ts`): function return type * refactor(`ApPersonService.ts`): type guardによるnon-null assertionの削除 * WIP(`ApPersonService.ts`): `analyzeAttachments` - Field型を事前に定義しておくように - `attachments`が`IObject`だった場合、返り値が`{ fields: [] }`になるようだが構わないのか? - `toArray()`を通すべきでは? * Revert "WIP(`ApImageService.ts`): `image.url`を`getApHrefNullable()`に通すかどうか悩んでいる" This reverts commit aeefb84. * cleanup(`ApImageService.ts`): `import` * refactor(`ApImageService.ts`): 冗長だった部分を短く * cleanup(`ApMentionService.ts`): `import` * refactor(`ApImageService.ts`): `JSON.stringify()`でのindentationを追加 * cleanup(`ApNoteService.ts`): `import` * cleanup(`ApNoteService.ts`) * cleanup(`ApNoteService.ts`) * cleanup(`ApNoteService.ts`) * cleanup(`ApNoteService.ts`): `any`に対するnon-null assertion * refactor(`ApNoteService.ts`): 添付ファイル * cleanup(`ApPersonService.ts`): `import` * refactor(`ApPersonService.ts`): より実情に即した`as`に * cleanup(`ApPersonService.ts`) * refactor(`ApPersonService.ts`): 冗長だった部分を修正 * cleanup(`ApPersonService.ts`): deadcode * cleanup(`ApPersonService.ts`) * cleanup(`ApQuestionService.ts`): `import` * refactor(`ApQuestionService.ts`): `eslint-disable-next-line` * refactor(`ApQuestionService.ts`): `eslint-disable-next-line` * cleanup(`ApQuestionService.ts`) * refactor(`ApQuestionService.ts`): non-null assertionを消した * cleanup(`ApQuestionService.ts`) * WIP(`ApQuestionService.ts`): non-null assertionを消す エラーメッセージを考える必要がある。 * refactor(`ApQuestionService.ts`): `any`を消す * refactor(`ApQuestionService.ts`): function return type * WIP(`ApPersonService.ts`): 可読性の低い三項演算子を削除しつつnon-null assertionを回避 エラーメッセージを考える必要がある。 * cleanup(`ApPersonService.ts`): 不必要な三項演算子を削除 * cleanup(`ApPersonService.ts`): 不要な`as` * cleanup(`ApPersonService.ts`) * refactor(`ApPersonService.ts`) * refactor(`ApPersonService.ts`): 可読性の低い三項演算子を削除 元の実装が悪いと判断し`null`かどうかの確認をより厳密に行うようにした。 * cleanup(`ApPersonService.ts`) * cleanup(`ApPersonService.ts`) * refactor(`ApPersonService.ts`): 返り値を`void`に統一 この返り値を参照しているコードは見当たらなかった。 また、普通に意味がない値であるように見受けられた。 * fixup! refactor(`ApPersonService.ts`): 返り値を`void`に統一 * refactor(`ApNoteService.ts`) * refactor(`ApPersonService.ts`) * cleanup(`ApPersonService.ts`) * cleanup(`ApPersonService.ts`) * refactor(`ApPersonService.ts`): 返り値の`void`統一と条件式の調整 この返り値を参照しているコードは見当たらなかった。 また、普通に意味がない値であるように見受けられた。 * cleanup(`ApQuestionService.ts`) * refactor(`ApQuestionService.ts`) * refactor(`ApQuestionService.ts`) * refactor(`tag.ts`): function return type * fixup! enhance: account migration (misskey-dev#10592) * fixup! WIP(`ApPersonService.ts`): 可読性の低い三項演算子を削除しつつnon-null assertionを回避 * fixup! cleanup(`ApPersonService.ts`): 不要な`as` * refactor: エラーメッセージを見繕った * Revert "cleanup(`ApImageService.ts`): `import`" This reverts commit 1454d04. * Revert "cleanup(`ApMentionService.ts`): `import`" This reverts commit 244f672. * Revert "cleanup(`ApNoteService.ts`): `import`" This reverts commit d8f0d76. * Revert "cleanup(`ApPersonService.ts`): `import`" This reverts commit 5190ef9. # Conflicts: # packages/backend/src/core/activitypub/models/ApPersonService.ts * Revert "cleanup(`ApQuestionService.ts`): `import`" This reverts commit 778585e. * processRemoteMoveはそのままにしてほしい * Revert "fixup! refactor(`ApPersonService.ts`): 返り値を`void`に統一" This reverts commit 083cd67. * Revert "refactor(`ApPersonService.ts`): 返り値を`void`に統一" This reverts commit bfa0fcd. --------- Co-authored-by: tamaina <[email protected]>
What
このPRでは
/packages/backend/src/core/activitypub/models/*.ts
を中心にリファクタリングをしました。Why
散見される
any
や型アサーションやnon-nullアサーションや関数の返り値の未定義などを、今後の円滑な開発のために取り除いておくべきだと考えました。any
type as possible #3622Additional info (optional)
現時点ではまだどう書くべきか決めかねている箇所があり、暫定的に「WIP」としてコミットしてあります。このPRのdraftを外す際にアドバイスを頂けたらうれしいです。ざっと見てみたところ、
core/activitypub/models/*.ts
に限らずcore/**/*.ts
にはリファクタリングが必要そうな箇所がいくつか見受けられました。しかしそちらまでこのPRに含めてしまうと流石に変更が膨大になってしまうと思われたため、このPRでは無視することにしました。Checklist