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

Features: Shuffle command, auto leave voice channel, bot status command. Fix: Play command #335

Closed
wants to merge 1 commit into from

Conversation

Uo1428
Copy link
Contributor

@Uo1428 Uo1428 commented Oct 5, 2024

@Uo1428 Uo1428 changed the title Done Features: Shuffle command, auto leave voice channel, bot status command. Fix: Play command Oct 5, 2024
@manuel-rw manuel-rw self-requested a review October 5, 2024 11:49
@manuel-rw manuel-rw added bug Something isn't working enhancement New feature or request labels Oct 5, 2024
@Uo1428
Copy link
Contributor Author

Uo1428 commented Oct 5, 2024

Q: in image
A: its done

image

)) as VoiceChannel;
// this.logger.log(voiceChannel?.name);
if (!voiceChannel) return;
const members = voiceChannel.members.filter((s) => !s.user?.bot);
Copy link
Owner

Choose a reason for hiding this comment

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

Rename s

const voiceChannel = (await mem.guild.channels.fetch(
this.voiceChannelId,
)) as VoiceChannel;
// this.logger.log(voiceChannel?.name);
Copy link
Owner

Choose a reason for hiding this comment

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

Remove commented out code or decrease log level

this.voiceConnection?.on(VoiceConnectionStatus.Disconnected, () => {
if (this.voiceConnection !== undefined) {
this.playbackService.getPlaylistOrDefault().clear();
this.disconnect();
}
});

const mem = member;
Copy link
Owner

Choose a reason for hiding this comment

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

Why?


const mem = member;
setTimeout(() => {
// leaving channel cuz no listener left
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// leaving channel cuz no listener left
// leaving channel because no listener is left

// this.logger.log(voiceChannel?.name);
if (!voiceChannel) return;
const members = voiceChannel.members.filter((s) => !s.user?.bot);
// this.logger.log(members.size);
Copy link
Owner

Choose a reason for hiding this comment

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

Remove commented out code or decrease log level

dmPermission: false,
})
@Injectable()
export class BotStatusCommand {
Copy link
Owner

Choose a reason for hiding this comment

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

Just a question, we don't want to persist the state anywhere?
And we also don't want to set the current song as the status?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find the constantly updating bot status a bit annoying, maybe the current song as status can be an option? Either way it should definitely be persistent.

Comment on lines +3 to +4
// bot status command params
// /bot-status [activity like playing, watching..] [indicator eg online, dnd..] [text]
Copy link
Owner

Choose a reason for hiding this comment

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

Not a valid JsDoc comment, fix or remove

export enum ClientPresenceStatus {
Online = 'online',
Idle = 'idle',
DoNotDisturb = 'dnd',
Copy link
Owner

Choose a reason for hiding this comment

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

Do not use abbreviations, use doNotDisturb as the enum value

await interaction.reply({
embeds: [
this.discordMessageService.buildErrorMessage({
title: 'tracks length is less than 2',
Copy link
Owner

Choose a reason for hiding this comment

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

Sentence start should be uppercase

Comment on lines +56 to +58
/**
** shuffle tracks
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Useless comment, document what this function actually does.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Randomize the order of tracks in the current playlist"

@manuel-rw
Copy link
Owner

manuel-rw commented Oct 5, 2024

For future reference, never push multiple features into the same branch or pull request.
It makes reviews harder and it will take longer until one specific feature is merged.
If you open multiple branches and PRs, then features will be merged faster and it is easier for both reviewer and developer.
I will reject PRs that do not meet this criteria in the future - thanks for you understanding.

See https://dev.to/amalv/a-cleaner-github-workflow-one-commit-per-pull-pequest-3ic1 , https://jun-sheng.medium.com/linear-git-history-part-i-b97184dde252, https://stackoverflow.com/questions/20348629/what-are-the-advantages-of-keeping-linear-history-in-git for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
3 participants