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

Fix client-server event schemas: move age, dedupe fields #1558

Merged
merged 14 commits into from
Aug 30, 2018

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Aug 25, 2018

Rendered: see 'docs' status check

Edit: Halfway through this PR the age field changed from being "removed" to just "moved" under unsigned.


This commit adds support for event schema examples to have references to help reduce the chance of fields being forgotten. This also helps reduce duplication of fields, allowing for a more consistent spec that uses the same values everywhere.

This also removes both unsigned and age from the examples as per:

Finally, this replaces "localhost" in the examples with an example domain. This is really just a nitpick thing on my part where seeing a "real world" domain is preferred.

Fixes #1524
Fixes #630
Step towards https://github.com/matrix-org/matrix-doc/issues/1530

This commit adds support for event schema examples to have references to help reduce the chance of fields being forgotten. This also helps reduce duplication of fields, allowing for a more consistent spec that uses the same values everywhere.

This also removes both `unsigned` and `age` from the examples as per:
* matrix-org#1524
* matrix-org#630

Finally, this replaces "localhost" in the examples with an example domain. This is really just a nitpick thing on my part where seeing a "real world" domain is preferred. 

Fixes matrix-org#1524
Fixes matrix-org#630
Step towards https://github.com/matrix-org/matrix-doc/issues/1530
@turt2live turt2live requested a review from a team August 25, 2018 00:06
@ara4n
Copy link
Member

ara4n commented Aug 25, 2018

hm, is this just removing age from the examples? or from the schema itself? as age as a param turned out to be very much required in the end (for WebRTC timeouts)

@turt2live
Copy link
Member Author

From the examples. I'll double check the schemas and make sure the age appears for the call event stuff.

@ara4n
Copy link
Member

ara4n commented Aug 27, 2018

i am still a bit confused about this - in practice, a compliant server is going to show an unsigned: { age: 1234 } block in all its /sync responses for a given event. Don't we want to show this in the examples for /sync responses etc (and not just for VoIP events)?

This is useful for a lot of things, like bridges (appservices), VoIP handling, and clients which generally may wish to do something with the field. Might as well include it on every event, despite the recommendation of matrix-org#1524
@turt2live turt2live changed the title Fix client-server event schemas: remove age, dedupe fields Fix client-server event schemas: move age, dedupe fields Aug 29, 2018
@turt2live turt2live requested a review from a team August 29, 2018 15:33
@turt2live
Copy link
Member Author

I've gone ahead and just added the unsigned field to everything, as per the rationale in 62b1b8b

This is primarily because application services (particularly bridges) have a very good reason to look at this field, alongside clients which do VoIP. It's worth noting that it is not a required field, however it is something that should be provided.

@turt2live
Copy link
Member Author

Fixes #1294
(now)

@turt2live turt2live mentioned this pull request Aug 29, 2018
11 tasks
Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

lgtm other than one nit

"content": {
"membership": "invite",
"avatar_url": "mxc://localhost/SEsfnsuifSDFSSEF#auto",
"avatar_url": "mxc://domain.com/SEsfnsuifSDFSSEF#auto",
"displayname": "Alice Margatroid"
},
"unsigned": {
Copy link
Member

Choose a reason for hiding this comment

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

the unsigned.age field gets lost here, which I'm guessing was not intentional

@turt2live turt2live merged commit 951b442 into matrix-org:master Aug 30, 2018
@turt2live turt2live deleted the travis/c2s/fix-events branch August 30, 2018 19:36
RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this pull request Aug 22, 2023
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.

4 participants