Skip to content
This repository has been archived by the owner on Jan 7, 2025. It is now read-only.

Add api docs for mutable message fields #44

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Conversation

SimonWoolf
Copy link
Member

...And update the descriptions of presence actions, which were not great

Which were not very good
descriptions.md Outdated

| Enum | Spec | Description |
|---|---|---|
| MESSAGE_UNSET | TM5 | (The field is unset, eg a `Message` retrieved from before the `action` field was added) |

Choose a reason for hiding this comment

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

In this case, I thought RT would set these to message.create when sending over the wire considering all messages before action was introduced are by definition a message.create? Or is that not the case?

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'm thinking here about history, realtime messages aren't an issue.

We could certainly say that every message with no action is implicitly a CREATE (and have the history api do that), but then the obvious question is, if that's the semantics we want then why do we even have an UNSET, why not make CREATE zero? @lmars you defined the enum, wdyt?

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 I originally added MESSAGE_UNSET with the assumption that Message.action would only be set for messages in a mutable messages namespace, but if we're always going to set it, then yes I think MESSAGE_CREATE should be the zero value.

descriptions.md Outdated Show resolved Hide resolved
descriptions.md Outdated Show resolved Hide resolved
descriptions.md Outdated Show resolved Hide resolved
descriptions.md Outdated Show resolved Hide resolved
descriptions.md Outdated Show resolved Hide resolved
@SimonWoolf SimonWoolf force-pushed the mutable-message-fields branch from 9dfa88b to 232996f Compare December 5, 2024 16:06
@lawrence-forooghian
Copy link
Collaborator

Adding @m-hulbert to review for style

@SimonWoolf SimonWoolf force-pushed the mutable-message-fields branch from 232996f to d7b8df9 Compare December 5, 2024 17:29
@SimonWoolf
Copy link
Member Author

Adding m-hulbert to review for style

Mark has indicated he does not think that the deved team need to review all new docstrings.

I think this process is clearly broken right now. The existence of this repo is not much known to anyone but the SDK team, who have been at some point instructed to update it in lockstep with the specification repo. The reason for it being its own repo instead of a file in the specification repo seems to have been lost to history, and the apparent original idea that it would be owned and maintained by the deved team no longer seems to be tenable given the current resources available.

There is certainly still value in having a file for shared docstrings between different sdks. But it should be a file in the specifications repo, where it can be versioned along with the spec, modified by the same pr that adds the same fields to the spec, and rendered to https://sdk.ably.com for the convenience of SDK devs.

I'm going to merge this pr after some final tweaks, then do that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants