Skip to content

Conversation

@mistermoe
Copy link
Contributor

2nd of 3 PRs to implement sync foundations. preceded by #290

Introduces an EventsGet message to retrieve events. Still working on tests for EventsGetHandler but i wanted to kick off the review process because it can be lengthy. I imagine there will be some discussion around MessageReply.

The PR that follows this one will introduce the MessagesGet interface method

@mistermoe mistermoe requested a review from dcrousso April 8, 2023 07:22
@mistermoe mistermoe linked an issue Apr 8, 2023 that may be closed by this pull request
@mistermoe
Copy link
Contributor Author

added handler tests

Copy link
Contributor

@thehenrytsai thehenrytsai left a comment

Choose a reason for hiding this comment

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

Looks pretty clean on DWN method implementation front, nothing much to say there! Have comments on MessageReply changes as you predicted.

dcrousso
dcrousso previously approved these changes Apr 14, 2023
diehuxx
diehuxx previously approved these changes Apr 15, 2023
Copy link

@diehuxx diehuxx left a comment

Choose a reason for hiding this comment

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

lgtm! thx for getting the changes in before Saturday, have a happy n healthy weekend :)

diehuxx
diehuxx previously approved these changes Apr 15, 2023
@frankhinek frankhinek self-requested a review April 15, 2023 06:53
frankhinek
frankhinek previously approved these changes Apr 15, 2023
Copy link
Contributor

@frankhinek frankhinek left a comment

Choose a reason for hiding this comment

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

LGTM based on the most recent changes

@mistermoe mistermoe dismissed stale reviews from frankhinek and diehuxx via 5a7da49 April 15, 2023 06:57
@mistermoe mistermoe merged commit 149c713 into main Apr 15, 2023
@mistermoe mistermoe mentioned this pull request Apr 16, 2023
@diehuxx diehuxx deleted the events-get branch June 23, 2023 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Sync

6 participants