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

Add VideoPost message type #145

Closed
wants to merge 17 commits into from

Conversation

su-chang
Copy link
Member

Related issue: #144

@su-chang su-chang requested a review from a team as a code owner August 30, 2021 09:26
windmemory
windmemory previously approved these changes Aug 30, 2021
Copy link
Member

@windmemory windmemory left a comment

Choose a reason for hiding this comment

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

LGTM

windmemory
windmemory previously approved these changes Aug 31, 2021
Copy link
Member

@windmemory windmemory left a comment

Choose a reason for hiding this comment

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

LGTM

src/schemas/channels.ts Outdated Show resolved Hide resolved
@su-chang
Copy link
Member Author

su-chang commented Sep 8, 2021

@huan ping

@su-chang su-chang changed the title Add channels message type Add VideoPost message type Sep 9, 2021
@windmemory
Copy link
Member

Shall we also add like and comments related method in this PR?

@su-chang
Copy link
Member Author

su-chang commented Sep 9, 2021

Shall we also add like and comments related method in this PR?

Here #149 , I will merge it into this PR later.

src/mod.ts Outdated
VideoPostPayload,
} from './schemas/video-post.js'
import type {
CommentPayload,
Copy link
Member

Choose a reason for hiding this comment

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

How about we reuse Message for comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean discard Comment class ? Or only reuse MessagePayload for CommentPayload?

Copy link
Member

Choose a reason for hiding this comment

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

If so, how do you think about it?

Copy link
Member Author

@su-chang su-chang Sep 10, 2021

Choose a reason for hiding this comment

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

src/puppet.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
export interface CommentPayload {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to consider reusing the Message payload first.

replyId is like we are quoting another message?

Discussion is welcome.

Copy link
Member Author

@su-chang su-chang Sep 10, 2021

Choose a reason for hiding this comment

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

Agree, show my draft design here:

  • add MessageType.Comment

  • add quoteId to MessagePayloadBase

  • methods change

    • commentContent() -> comment() // differ with message.say()
    • commentRevoke() -> delete() // differ with message.recall()
    • commentReply() -> reply()

@@ -0,0 +1,4 @@
export interface ListOption {
pageSize: number,
currentPage: number,
Copy link
Member

Choose a reason for hiding this comment

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

According to the Google Cloud API Design guides, where should we put the next_page_token in this payload?

Copy link
Member Author

@su-chang su-chang Sep 10, 2021

Choose a reason for hiding this comment

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

Don't agree, next_page_token is in response object. Use pageToken replace currentPage.

And then I would add startAfter, it could make get list more flexibly.

interface ListOption {
  pageSize: number,
  pageToken?: number,
  startAfter?: string,
}

Copy link
Member

Choose a reason for hiding this comment

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

startAfter is not a good name, because the list should be a [start, end) range.

To include the start number, the name of startFrom would be better than startAfter.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to get the list which start from one specific KEY, how to do with ListOption?

For example, Moment timeline, when we want to get the next page, we should specific the last moment id as the KEY.

Due to the moment timeline will have new moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Question about the type of startAfter

the list should be a [start, end) range.

  • number

    Vote for rename it to startFrom.

  • string

    Vote for keep startAfter, due to we only known the startAfter key.

src/schemas/video-post.ts Outdated Show resolved Hide resolved
tests/fixtures/puppet-test/puppet-test.ts Outdated Show resolved Hide resolved
@huan
Copy link
Member

huan commented Jan 22, 2022

Merge to #182

@huan huan added the duplicate This issue or pull request already exists label Jan 22, 2022
@huan huan closed this Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants