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.

Sign up for GitHub

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

Jump to bottom

docs: improve documentation #90

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

docs: improve documentation #90

wants to merge 5 commits into from

Conversation

NotSugden
Copy link

@NotSugden NotSugden commented Sep 16, 2020 •
edited
Loading

This PR adds some better documentation, corrects some invalid syntax in the JSDoc comments, and deprecates RPCClient#sendJoinRequest as the command has been removed

this PR only changes documentation

reference: this pr is apart of the chain of prs requested from #89 (comment)

@NotSugden NotSugden marked this pull request as draft September 16, 2020 20:24
@NotSugden NotSugden marked this pull request as ready for review September 16, 2020 21:30
devsnek
devsnek reviewed Sep 17, 2020
View reviewed changes
src/constants.js Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
src/client.js Show resolved Hide resolved
src/client.js Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
devsnek
devsnek reviewed Sep 17, 2020
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
papaia
papaia suggested changes Sep 17, 2020
View reviewed changes
Copy link

@papaia papaia left a comment

Choose a reason for hiding this comment

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

As per JSDoc docs

image

src/constants.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
@NotSugden
Copy link
Author

Updated to requested changes, however there is just a nit - the VoiceSettings and UserVoiceSettings typedefs have properties marked as optional (for editing via any of the respective set* methods), but the same type is used as the return type - this would be incorrect as the properties will not be optional, not really sure of the best way to go about that or if it should even be addressed

@papaia
Copy link

papaia commented Sep 17, 2020

Shouldn't Promise<undefined> be changed to Promise<void>?

@NotSugden
Copy link
Author

Shouldn't Promise<undefined> be changed to Promise<void>?

thats what i thought, but #90 (comment)

@NotSugden NotSugden mentioned this pull request Sep 17, 2020
Open
@NotSugden NotSugden requested a review from devsnek September 18, 2020 20:42
@dscalzi
Copy link
Contributor

dscalzi commented Sep 21, 2020

Could throw in an index.d.ts while you're at it.

@NotSugden
Copy link
Author

Could throw in an index.d.ts while you're at it.

see #89 (comment) snek doesn't want typings in the library itself

noftaly
noftaly reviewed Feb 8, 2021
View reviewed changes
src/client.js Show resolved Hide resolved
@NotSugden NotSugden marked this pull request as draft March 10, 2021 21:06
@NotSugden NotSugden marked this pull request as ready for review March 10, 2021 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noftaly noftaly noftaly left review comments

@papaia papaia papaia requested changes

@devsnek devsnek Awaiting requested review from devsnek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NotSugden @papaia @dscalzi @devsnek @noftaly