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

Improve JSDocs to be more descriptive and better overall #101

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Improve JSDocs to be more descriptive and better overall #101

merged 1 commit into from
Mar 19, 2024

Conversation

Reishimanfr
Copy link
Contributor

I've improved all relevant JSDocs across files Player.ts, Poru.ts, Queue.ts, and Response.ts so they have a more descriptive JSDoc comment. I also fixed some spelling issues/sentences that made no sense.

Breaking changes:

  • I've changed the player.stop method to player.skip instead as stop implies a complete stop of the player's playback where skip implies skipping the currently playing song
  • I've removed the unnecessary parameter from poru.init (that being the client parameter) as it's already provided in poru's constructor

import { Filters } from "./Filters"
import { Response, LoadTrackResponse } from "../guild/Response"

type Loop = "NONE" | "TRACK" | "QUEUE"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was the jsdoc removed of loop type?

* @param {Node | undefined} node Node or undefined
* @returns {Promise<Response>} The Response of the resolved tracks
* Resolves a track.
* @param {ResolveOptions} options - Options for resolving tracks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably an invalid param as there no param called as options defined in the resolve function

@UnschooledGamer
Copy link
Collaborator

I've improved all relevant JSDocs across files Player.ts, Poru.ts, Queue.ts, and Response.ts so they have a more descriptive JSDoc comment. I also fixed some spelling issues/sentences that made no sense.

Breaking changes:

  • I've changed the player.stop method to player.skip instead as stop implies a complete stop of the player's playback where skip implies skipping the currently playing song
  • I've removed the unnecessary parameter from poru.init (that being the client parameter) as it's already provided in poru's constructor

Overall looks Good, Discussing about the breaking changes firstly the player.stop

"implies a complete stop of the player's playback"

true, that's actually what it does completely stops the current playing track.
secondly client parameter is present probably to provide the updated client data as <Poru>.init would be run once the discord client is ready(fully connected to discord) and before that client won't have access to it's user id.

I might be wrong don't mind it and correct me.

Apart from this I'll wait 24h for @parasop to review if he doesn't in 24h I'll merge this. (If I don't or forgot to do so Kindly Reminder me 😅)

-- Thanks UnschooledGamer

@UnschooledGamer UnschooledGamer requested a review from parasop March 19, 2024 13:45
@Reishimanfr
Copy link
Contributor Author

Reishimanfr commented Mar 19, 2024

secondly client parameter is present probably to provide the updated client data as .init would be run once the discord client is ready(fully connected to discord) and before that client won't have access to it's user id

The thing about the client (at least for discord.js, not sure about other libraries) is that you can't access the user property if it's not initialized. I didn't see any other references to the client parameter inside of the init method which made me think it's superfluous.

@Reishimanfr
Copy link
Contributor Author

true, that's actually what it does completely stops the current playing track.

As a user of the library I avoided the stop method as I figured it would completely stop the player and disconnect it from the voice channel similar to what destroy does hence why I renamed it to a more straight-forward skip instead. It may just be me but I feel like skip would work better here.

@UnschooledGamer
Copy link
Collaborator

secondly client parameter is present probably to provide the updated client data as .init would be run once the discord client is ready(fully connected to discord) and before that client won't have access to it's user id

The thing about the client (at least for discord.js, not sure about other libraries) is that you can't access the user property if it's not initialized. I didn't see any other references to the client parameter inside of the init method which made me think it's superfluous.

It's most likely the same as after the bot successfully connects discord Gateway it returns the information about the user id etc.

@Reishimanfr
Copy link
Contributor Author

Probably yeah. So the client parameter seems to be redundant here then.

@UnschooledGamer
Copy link
Collaborator

true, that's actually what it does completely stops the current playing track.

As a user of the library I avoided the stop method as I figured it would completely stop the player and disconnect it from the voice channel similar to what destroy does hence why I renamed it to a more straight-forward skip instead. It may just be me but I feel like skip would work better here.

stop just tells Lavalink to stop the current playing song that's all. The disconnection afterwards is probably by code written somewhere.

@Reishimanfr
Copy link
Contributor Author

true, that's actually what it does completely stops the current playing track.

As a user of the library I avoided the stop method as I figured it would completely stop the player and disconnect it from the voice channel similar to what destroy does hence why I renamed it to a more straight-forward skip instead. It may just be me but I feel like skip would work better here.

stop just tells Lavalink to stop the current playing song that's all. The disconnection afterwards is probably by code written somewhere.

I'm not saying stop disconnects the bot from the voice channel, just saying that's what it feels like it would do

@UnschooledGamer
Copy link
Collaborator

true, that's actually what it does completely stops the current playing track.

As a user of the library I avoided the stop method as I figured it would completely stop the player and disconnect it from the voice channel similar to what destroy does hence why I renamed it to a more straight-forward skip instead. It may just be me but I feel like skip would work better here.

stop just tells Lavalink to stop the current playing song that's all. The disconnection afterwards is probably by code written somewhere.

I'm not saying stop disconnects the bot from the voice channel, just saying that's what it feels like it would do

Ah, doesn't feel for me. stop means it stops the player and destroy means it destroys the player.

@Reishimanfr
Copy link
Contributor Author

Still, I feel like skip is just a better name for a method if it's supposed to skip the currently playing song and play the next one in the queue like normal

@parasop parasop merged commit 9740ef1 into parasop:v5 Mar 19, 2024
1 of 2 checks passed
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.

3 participants