Skip to content

WIP: Cleaning code (See description)#1

Open
MrJacz wants to merge 13 commits into
briantanner:masterfrom
MrJacz:master
Open

WIP: Cleaning code (See description)#1
MrJacz wants to merge 13 commits into
briantanner:masterfrom
MrJacz:master

Conversation

@MrJacz
Copy link
Copy Markdown
Contributor

@MrJacz MrJacz commented Jan 1, 2018

Done:
PlayerManager#Join function returns promise off the bat

Todo:
Better error handling and type checking

@MrJacz MrJacz requested a review from briantanner January 1, 2018 10:30
@briantanner
Copy link
Copy Markdown
Owner

I don't think there's any need for a PlayerManager.add method, the only reason it existed in eris-lavalink was because it was extending Eris.Collection and there was some code reuse, by adding that method you'll only save 1 line of code.

Also, regarding _ private methods, I think the @private distinction in the jsdoc should be enough.

@MrJacz
Copy link
Copy Markdown
Contributor Author

MrJacz commented Jan 1, 2018

Im not taking the idea for the add method from Eris#Collection but its would be handy considering then

      player = player || new this.baseObject(data.guild_id, { // eslint-disable-line
        client: this.client,
        guildId: data.guild_id,
        sessionId: data.session_id,
        channelId: this.pendingGuilds[data.guild_id].channelId,
        hostname: this.pendingGuilds[data.guild_id].hostname,
        node: this.pendingGuilds[data.guild_id].node,
        options: this.pendingGuilds[data.guild_id].options,
        event: data,
        manager: this
      });

      this.set(data.guild_id, player);

could be reduced to

player = player || this.add(data here);

and for the _ private methods, I do agree

@briantanner
Copy link
Copy Markdown
Owner

We'll never use that method again, just to save one line of code.

@MrJacz MrJacz changed the base branch from master to develop January 1, 2018 13:31
@MrJacz MrJacz changed the base branch from develop to master January 1, 2018 13:35
@MrJacz MrJacz mentioned this pull request Jan 5, 2018
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.

2 participants