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

Threads: The base #1254

Merged
merged 6 commits into from
Sep 27, 2022
Merged

Threads: The base #1254

merged 6 commits into from
Sep 27, 2022

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Sep 27, 2022

Specifies matrix-org/matrix-spec-proposals#3440 (minus filtering requirements due to MSC3856)
Specifies matrix-org/matrix-spec-proposals#3856
Specifies matrix-org/matrix-spec-proposals#3715

This is reviewable commit-by-commit, for each of the MSCs listed above.

The remainder of the threads work (notifications, read receipts) is handled by #1255.

Requires matrix-org/matrix-spec-proposals#3899 for clarifications.

Preview: https://pr1254--matrix-spec-previews.netlify.app

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Words look good. I defer to @clokep about whether this matches reality (though AIUI it does)

Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Examples look reasonable to me, just a couple of minor points.

content/client-server-api/modules/threading.md Outdated Show resolved Hide resolved
Comment on lines +181 to +182
`latest_event` is the most recent event (topologically to the server) in the thread sent by an
un-[ignored user](#ignoring-users).
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it needs to be called out, but the bundled latest_event should be serialized using the same format as the event itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done via the spec and examples implicitly: the other weird formats aren't in the spec anymore, as that's a legacy implementation detail in some clients and servers.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I hadn't realized those were no longer in the spec. Please ignore me then!

Copy link

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

lgtm


As a worked example, the following represents a thread and how it'd be formed:

```json

Choose a reason for hiding this comment

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

Should we use json5 or jsonc to highlight syntax here and in other code examples?

Copy link
Member Author

Choose a reason for hiding this comment

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

json because hugo does a better job than github, annoyingly.

clokep added a commit to clokep/matrix-spec that referenced this pull request May 3, 2023
* Spec MSC3440: Threading (just the base)

Other threading MSCs to follow

* Spec MSC3856: Threads list API

* Spec MSC3715:  Add`dir` to `/relations`

* changelog

* Apply suggestions from code review

Co-authored-by: Patrick Cloke <[email protected]>

* Update changelogs/client_server/newsfragments/1254.feature

Co-authored-by: Patrick Cloke <[email protected]>

Co-authored-by: Patrick Cloke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Blocks the next release from happening
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants