-
Notifications
You must be signed in to change notification settings - Fork 379
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
General/small improvements to the application service API specification #1533
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm modulo some minor quibbles
@@ -32,6 +32,10 @@ paths: | |||
description: |- | |||
This API is called by the homeserver when it wants to push an event | |||
(or batch of events) to the application service. | |||
|
|||
The application service should take care to ensure that it handles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/handles/detects/
or 'Note that the application service should distinguish state events from message events via the presence of a state_key
, rather than via the event type."
@@ -217,7 +224,8 @@ need to be able to adjust the ``origin_server_ts`` value to do this. | |||
|
|||
Inputs: | |||
- Application service token (``as_token``) | |||
- Desired timestamp | |||
- Desired timestamp in milliseconds since the unix epoch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest using parens: "Desired timestamp (in milliseconds since the unix epoch)."
(Aside: should we just have a note in the introduction that all timestamps are in ms since the epoch, and then just use 'timestamp' throughout the rest of the spec?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's #1468 that kinda covers the case of "wtf is a timestamp". Putting it in the intro seems much better than going through the entire spec looking for "timestamp".
@@ -39,7 +40,7 @@ This version of the specification is generated from | |||
Application Services | |||
-------------------- | |||
Application services are passive and can only observe events from a given | |||
homeserver. They can inject events into rooms they are participating in. | |||
homeserver (HS). They can inject events into rooms they are participating in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd kinda rather we just used 'homeserver' throughout. But I don't mind overmuch if you don't want to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also rather "homeserver", however the diff becomes insane because "HS" is used everywhere, pushing the line length all out of whack. Kinda settled for this pending something to take out the acronyms from everywhere as one giant "fix the world" PR or something.
Rendered: see 'docs' status check
Explicitly say how appservices should detect state events
Fixes #1014
Misc language changes
Clearly state how the users namespace relates to interest in events
Fixes #1307