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

4 participants
@turt2live
Member

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 #1530

Fix client-server event schemas: remove `age`, dedupe fields
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:
* #1524
* #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 #1524
Fixes #630
Step towards #1530

@turt2live turt2live added this to In review (just the PRs) in August 2018 r0 via automation Aug 25, 2018

@turt2live turt2live requested a review from matrix-org/spec-core-team Aug 25, 2018

@ara4n

This comment has been minimized.

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

This comment has been minimized.

Member

turt2live commented Aug 25, 2018

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

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Aug 26, 2018

@ara4n

This comment has been minimized.

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)?

turt2live added some commits Aug 28, 2018

Have unsigned.age appear on all room 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 #1524

@turt2live turt2live changed the title from Fix client-server event schemas: remove `age`, dedupe fields to Fix client-server event schemas: move `age`, dedupe fields Aug 29, 2018

@turt2live turt2live requested a review from matrix-org/spec-core-team Aug 29, 2018

@turt2live

This comment has been minimized.

Member

turt2live commented Aug 29, 2018

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

This comment has been minimized.

Member

turt2live commented Aug 29, 2018

Fixes #1294
(now)

@turt2live turt2live referenced this pull request Aug 29, 2018

Closed

r0 for the Application Services API #1333

10 of 11 tasks complete

@turt2live turt2live moved this from Reviewer approved to In review (just the issues) in August 2018 r0 Aug 29, 2018

@turt2live turt2live moved this from In review (just the issues) to In review (just the PRs) in August 2018 r0 Aug 29, 2018

turt2live added some commits Aug 30, 2018

Don't check the underlying definitions
Otherwise the script will try to find a schema for our templates, which don't exist.

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Aug 30, 2018

@uhoreg

uhoreg approved these changes Aug 30, 2018

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": {

This comment has been minimized.

@uhoreg

uhoreg Aug 30, 2018

Member

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

7 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

August 2018 r0 automation moved this from Reviewer approved to Done (this list will be incomplete) Aug 30, 2018

@turt2live turt2live deleted the turt2live:travis/c2s/fix-events branch Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment