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

Clarify terminology around aggregations #1424

Merged
merged 4 commits into from Mar 21, 2023
Merged

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jan 31, 2023

I've done my best to remove the word "bundle", because I feel like it causes more confusion than clarification. Instead I have favoured "aggregated child events" which I think is clearer.

Some general clarification around these parts of the spec.

Hopefully, fixes #1405

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

@richvdh richvdh requested a review from a team as a code owner January 31, 2023 19:56
I've done my best to remove the word "bundle", because I feel like it causes
more confusion than it provides. Instead I have favoured "aggregated child
events" which I think is clearer.

Some general clarification around these parts of the spec.
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Generally this lgtm with a couple (hopefully easy) comments to resolve.

@@ -32,8 +32,7 @@ paths:
x-addedInMatrixVersion: "1.4"
summary: Retrieve a list of threads in a room, with optional filters.
description: |-
Paginates over the thread roots in a room, ordered by the `latest_event` of each thread root
in its bundle.
Paginates over the thread roots in a room.
Copy link
Member

Choose a reason for hiding this comment

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

The description appears to have lost some context here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seemed redundant to specify the sort order here as well as in the response.

Copy link
Member

Choose a reason for hiding this comment

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

it adds a bit of clarity without having to scroll past a whole request schema, though not all that bothered.

The summary and description should be switched if we are losing the text though: it's more awkward if the summary is longer than the description.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have updated a bit more, with reference to https://github.com/matrix-org/matrix-spec/blob/main/meta/documentation_style.rst#openapi.

If you still don't like it, am happy to put it back how it was.

meta/documentation_style.rst Outdated Show resolved Hide resolved
@richvdh richvdh requested a review from turt2live March 1, 2023 10:30
@richvdh richvdh merged commit d26794e into main Mar 21, 2023
10 checks passed
@richvdh richvdh deleted the rav/clarify_aggregations branch March 21, 2023 18:27
clokep pushed a commit to clokep/matrix-spec that referenced this pull request May 3, 2023
I've done my best to remove the word "bundle", because I feel like it causes
more confusion than it provides. Instead I have favoured "aggregated child
events" which I think is clearer.

Some general clarification around these parts of the spec.
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.

Aggregations are confusing
2 participants