Skip to content
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

feat: implement API of timeline #239

Closed
wants to merge 1 commit into from
Closed

feat: implement API of timeline #239

wants to merge 1 commit into from

Conversation

nanai10a
Copy link
Member

What does this PR do?

NONWRITTEN

Additional information

NONWRITTEN

  • 先頭のコミットは消し飛びます

@nanai10a nanai10a added the T: feature New feature label Mar 28, 2024
@nanai10a nanai10a requested a review from laminne March 28, 2024 02:31
@nanai10a nanai10a self-assigned this Mar 28, 2024

import { accounts } from './pkg/accounts/mod.js';
import { noteHandlers } from './pkg/notes/mod.js';

ihp.installIntoGlobal();
Copy link
Member

Choose a reason for hiding this comment

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

(実際にPR出すときはこれ入れた理由をお願いしますね)

export type NoteFilter =
| { type: 'author'; any: ID<AccountID>[] }
| { type: 'attachment'; more: number }
| { type: 'cw'; is: string }
Copy link
Member

Choose a reason for hiding this comment

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

Note.ContentsWarningCommentはあればContentsWarning扱いされるものなので、この書き方だとCWCommentの完全一致検索になってしまって意図しない挙動になるかと

Copy link
Member Author

Choose a reason for hiding this comment

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

主題から少し逸れますが, "contents warning comment" はどのように表記するのが正解なのでしょうか? CwComment のような略も存在していたと思いますが, ここでは "CW(C)" と頭文字まで切り詰めています. というかそもそも少し長すぎやしませんか?より簡潔な表現があると良いような…

}
}

return Result.ok(notes.take(NOTES_LIMIT).toArray());
Copy link
Member

Choose a reason for hiding this comment

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

時刻でソートしてからLimitしたほうが良い気がします

| { type: 'attachment'; more: number }
| { type: 'cw'; is: string }
| { type: 'created'; less: Date }
| { type: 'updated'; less: Date }
Copy link
Member

Choose a reason for hiding this comment

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

updatedはタイムラインでは必要ないかと.

Copy link
Member Author

Choose a reason for hiding this comment

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

如何なるタイムラインでも更新日時は並び順に一切考慮されない (考慮されるのは投稿日時のみ) ということで良いですよね?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, 現時点ではそうです

findByAuthorID(
authorID: ID<AccountID>,
limit: number,
): Promise<Option.Option<Note[]>>;
findByID(id: ID<NoteID>): Promise<Option.Option<Note>>;
deleteByID(id: ID<NoteID>): Promise<Result.Result<Error, void>>;
}

export type NoteFilter =
| { type: 'author'; any: ID<AccountID>[] }
Copy link
Member

Choose a reason for hiding this comment

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

複数のユーザーの投稿をフィルターしたいユースケースが浮かなばなかったのですが、そういうユースケースがあるんでしょうか?

Copy link
Member Author

Choose a reason for hiding this comment

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

ここでは投稿者で投稿をフィルタしているので, タイムラインに流す投稿は 指定したアカウントがフォローしているアカウント全ての投稿 に絞る, という手法を取っています. 指定したアカウントとは, つまり /timeline/home では認証情報の主であり, /timeline/accounts/:spec では :spec が該当し, /timeline/global は フィルタ無し (全投稿者による投稿の集合) となります.

Comment on lines +17 to +19
private readonly fetchAccountService: FetchAccountService,
private readonly fetchAccountFollowService: FetchAccountFollowService,
private readonly fetchNoteService: FetchNoteService,
Copy link
Member

Choose a reason for hiding this comment

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

これらはintermoduleモジュールからimportしたものを使ってください

Copy link
Member Author

Choose a reason for hiding this comment

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

AccountModule は既にあるものを (必要に応じて) 拡張するとして, NoteModule は別で立てる必要がありますね?

Copy link
Member

Choose a reason for hiding this comment

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

そうしてください

? await this.fetchAccountFollowService.fetchFollowsByID(target)
: isAccountName(target)
? await this.fetchAccountFollowService.fetchFollowsByName(target)
: panic();
Copy link
Member

Choose a reason for hiding this comment

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

throwするよりResult.errを返した方が良いかと

const isID = <T>(s: string): s is ID<T> => {
return z
.string()
.regex(/^\d{64}$/)
Copy link
Member

Choose a reason for hiding this comment

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

Pulsate-SnowflakeIDは固定長ではないのでこれは落ちると思います

Copy link
Member Author

@nanai10a nanai10a Mar 28, 2024

Choose a reason for hiding this comment

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

Zod によるパーサ (少なくとも型判定ができるもの) で少なくとも ID<T>AccountName についてのパーサが欲しいのですが, 既知の実装か formal な規格が存在しますか?

Copy link
Member

@laminne laminne Mar 28, 2024

Choose a reason for hiding this comment

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

どちらも既知の実装はありません
仕様に関してはIDはreadmeに書いてあります

Copy link
Member Author

Choose a reason for hiding this comment

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

ID<T> に関して了解しました. AccountName に関して該当する仕様が見つけられなかったのですが, ご教示願えますでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

Notionに記載があります

@nanai10a nanai10a changed the title DUMP CODES feat: タイムライン API の実装 Mar 29, 2024
@nanai10a nanai10a changed the title feat: タイムライン API の実装 feat: implement API of timeline Mar 29, 2024
@laminne
Copy link
Member

laminne commented Jul 21, 2024

#428 で実装済み

@laminne laminne closed this Jul 21, 2024
@m1sk9 m1sk9 deleted the feat/dev-15 branch July 21, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants