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

Update presence types & documentation #267

Merged
merged 1 commit into from Oct 11, 2022
Merged

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Oct 8, 2022

Checklist

  • Tests written for all new code
  • Linter has been satisfied
  • Sign-off given on the changes (see CONTRIBUTING.md)

While working on a PR for matrix-appservice-discord, I was tripped up by the presence states.
I usually only look in the JSDocs for information while working on projects/PRs, and the term unavailable wasn't clearly defined or linked to before.

Looked to me that "unavailable" was either:

  • Home server has presence disabled, so the presence is unavailable.
  • DND

I had to ask in chat to find out it was actually neither of those things. ^-^'

Maybe for convenience for myself and others, could we just add a link to the spec that clearly defines these?

@turt2live
Copy link
Owner

Unfortunately the spec isn't the most helpful resource for who this SDK is trying to target: beginner or near-beginner bot writers (obviously still supporting the more advanced users too). The SDK is far from that point currently, but I'd like to avoid spec links for now.

If there's a guide which can be linked to (or created), that would be preferable

@SethFalco
Copy link
Contributor Author

SethFalco commented Oct 9, 2022

Unfortunately the spec isn't the most helpful resource for who this SDK is trying to target

Tbh, I thought the spec did a pretty good job in this case at least. Though I'll admit the spec can be intimidating to check out.

However, if we're keen on not putting spec links for now, I could try and find a guide, or just duplicate some of the information over as JSDocs.

@turt2live
Copy link
Owner

A guide or duplicating would be fine, yea

Signed-off-by: Seth Falco <seth@falco.fun>
@SethFalco
Copy link
Contributor Author

SethFalco commented Oct 9, 2022

I couldn't find a guide for it, so opted to duplicate.
Probably depends on the processor, but VSC renders Markdown in JSDocs so just did it in Markdown.

image

image

Also added the @see in PresenceEventContent#presence since it's more often going to be exposed through here. This one might be silly, technically a user could just click through on their own anyway.

While making this change, I noticed this was actually already documented in MatrixClient#syncingPresence. I just never ran into that while consuming the SDK in matrix-appservice-discord. However, the one in MatrixClient also covers null, so I guess it's technically different.


On the side, I noticed the type "online" | "offline" | "unavailable" was done in multiple places. Is there any reason we couldn't just have PresenceState used in those places? I updated it in this PR, but please let me know if that should be undone.

@turt2live turt2live self-requested a review October 10, 2022 07:07
@turt2live turt2live changed the title docs: add link to presence spec in jsdoc Update presence types & documentation Oct 11, 2022
Copy link
Owner

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thanks :)

@turt2live turt2live merged commit 8820068 into turt2live:main Oct 11, 2022
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.

None yet

2 participants