-
-
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): replace punyHost
with new URL().host
#15164
base: develop
Are you sure you want to change the base?
Conversation
このPRによるapi.jsonの差分 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #15164 +/- ##
===========================================
- Coverage 39.95% 39.94% -0.01%
===========================================
Files 1563 1563
Lines 197878 197869 -9
Branches 3635 3631 -4
===========================================
- Hits 79059 79047 -12
- Misses 118215 118217 +2
- Partials 604 605 +1 ☔ View full report in Codecov by Sentry. |
別に用意されていたということはなにか事情があった可能性があるので、URL.hostと完全に等価なものかどうかを確認する必要がありそう |
https://url.spec.whatwg.org/#host-serializing によると ASCII String (punycode化された文字列)が返されることになっているので、等価なものとして扱っていいと思います |
MisskeyIO#858 のほうが良いかも |
|
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.
実装に問題は無いと思うのでApproveします!
…が、少々に気になった点があります。感覚的な要素が強いので無視していただいても構いません。
if (new URL(url).host !== new URL(note.id).host) {
の表記は、new、スペース、大文字の連続、パラメータ、newで作成されるオブジェクトのプロパティ…というように、全体的な文字数は減っているものの、かえって複雑になっている印象を受けます。
なので、utilityService.punyHost
の関数定義はそのまま残しておき、この関数の中身を
public punyHost(url: string): string {
new new URL(url).host;
}
のようにするのはいかがでしょうか…?
What
タイトル通り
Closes #15163
Why
コード量を減らすため
Additional info (optional)
Checklist