Skip to content

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Sep 8, 2025

MSC: matrix-org/matrix-spec-proposals#4311

Reviewable(ish) commit-by-commit.

Note: In line with existing spec, this change reminds readers constantly about the event format being different depending on room version.

Pull Request Checklist

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

@turt2live turt2live changed the title Travis/spec 4311 Specification for MSC4311: Create invite availability in stripped state Sep 8, 2025
@turt2live turt2live marked this pull request as ready for review September 8, 2025 22:26
@turt2live turt2live requested a review from a team as a code owner September 8, 2025 22:26
@turt2live turt2live added the release-blocker Blocks the next release from happening label Sep 8, 2025
@turt2live turt2live mentioned this pull request Sep 8, 2025
15 tasks
Comment on lines +74 to +76
x-changedInMatrixVersion:
"1.16": |
`m.room.create` and format requirements were added.
Copy link
Member

Choose a reason for hiding this comment

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

(I am belatedly wishing that we had just deprecated /v1/invite and mandated that everyone uses /v2, rather than adding more changes to /v1.)

Do you think it's worth trying to do some $ref trickery to reduce the duplication in the yaml, at least?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been bitten by $ref not working in weird and unpredictable ways in the past, so I'm incentivized to copy/paste. The endpoints are also slightly different in error responses, which makes it especially annoying.

If given a choice, I'd rather not fight $ref more than I already have to.

Copy link
Member

Choose a reason for hiding this comment

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

I think that @zecakeh has done a bunch of work to make $ref work better, so it might be worth taking another look.

The endpoints are also slightly different in error responses, which makes it especially annoying.

Ok, but you don't have to duplicate the whole endpoint -- you can just share some parameter type definitions, for example.

I'm really not a big fan of having two copies of all this stuff - it makes it much harder to maintain and review. Doing this review was particularly annoying because I kept getting confused about which bits I'd seen and which I hadn't. Making the job harder for the reviewer to save a bit of authoring time is not a great investment.

I'll not insist on it this time around though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The concern with the error responses is that it's harder to deduplicate that part is all - the request parameters can be deduplicated, but that's only so helpful.

I'll experiment with $ref separate from this, and open a PR if it works.

@turt2live turt2live requested a review from richvdh September 12, 2025 02:50
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

A few things I'd still like to see cleaned up, but no blockers here from me

Comment on lines +74 to +76
x-changedInMatrixVersion:
"1.16": |
`m.room.create` and format requirements were added.
Copy link
Member

Choose a reason for hiding this comment

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

I think that @zecakeh has done a bunch of work to make $ref work better, so it might be worth taking another look.

The endpoints are also slightly different in error responses, which makes it especially annoying.

Ok, but you don't have to duplicate the whole endpoint -- you can just share some parameter type definitions, for example.

I'm really not a big fan of having two copies of all this stuff - it makes it much harder to maintain and review. Doing this review was particularly annoying because I kept getting confused about which bits I'd seen and which I hadn't. Making the job harder for the reviewer to save a bit of authoring time is not a great investment.

I'll not insist on it this time around though.

@turt2live turt2live changed the title Specification for MSC4311: Create invite availability in stripped state Specification for MSC4311: Create event availability in stripped state Sep 12, 2025
@turt2live turt2live merged commit 7bc016b into main Sep 12, 2025
12 checks passed
@turt2live turt2live deleted the travis/spec-4311 branch September 12, 2025 19:47
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

None yet

Development

Successfully merging this pull request may close these issues.

2 participants