Skip to content
This repository has been archived by the owner on Nov 13, 2022. It is now read-only.

Selection of the ideal Lavalink node according to the location of the Discord server #88

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

Conversation

karelkryda
Copy link

It is quite likely that there will be some errors in the code. I wrote this code in JavaScript and then transcribed it into TypeScript, a language I don't quite understand.

The principle is that I added the "region" option to the node options, which can be either a string or a array of strings.
Furthermore, the Player now also has the "region" option, which is the region of the Discord server (string).
If the user specifies the Node he wants to use when creating the Player, that one is selected (as before).
If the user has provided the "region" option, the corresponding Nodes are selected.

  • If none is found, null is returned
  • If one matching Node is found, it will return
  • If more than one matching Nodes are found, the best one will be selected according to the statistics of the given Node.

If no Node is returned or the "region" option is not provided, the best one is selected according to the statistics of that Node (as before)

@acollierr17
Copy link
Contributor

Has this code been tested?

@karelkryda
Copy link
Author

karelkryda commented Mar 9, 2021

Has this code been tested?

yeah, I tested it - but in JavaScript => idk if I rewrote it to TypeScript correctly

@JonasSchiott
Copy link
Member

Has this code been tested?

yeah, I tested it - but in JavaScript => idk if I rewrote it to TypeScript correctly

The thing you gotta realise is, Erela is our baby, hehe. We'd prefer it to be without bugs, and your codebase and amount of changes are almost too big to get a grasp of in one sitting. When we ask if your code has been tested, you respond with "only before, when I wrote it in a language I understand." What do we do with this? I suppose @acollierr17 & myself are forced to do quite some testing with this to avoid bugs if possible.

@karelkryda
Copy link
Author

Has this code been tested?

yeah, I tested it - but in JavaScript => idk if I rewrote it to TypeScript correctly

The thing you gotta realise is, Erela is our baby, hehe. We'd prefer it to be without bugs, and your codebase and amount of changes are almost too big to get a grasp of in one sitting. When we ask if your code has been tested, you respond with "only before, when I wrote it in a language I understand." What do we do with this? I suppose @acollierr17 & myself are forced to do quite some testing with this to avoid bugs if possible.

I've already dealt with someone from you from Discord that I don't know much about TS. That man told me that someone would help me with the transport from JS to TS. I tried to make the code correct. Respectively, the code should be functional, I'm just not sure about the types of variables in the return and input methods.

there were a lot of commits, since I already made one pull request. Unfortunately, I was unable to reset changes to the repository. So that's the reason for so many commits

@anishshobithps anishshobithps added the enhancement New feature or request label Apr 19, 2021
@anishshobithps anishshobithps added this to the v3 milestone Apr 19, 2021
@@ -119,7 +124,9 @@ export class Player {
if (options.textChannel) this.textChannel = options.textChannel;

const node = this.manager.nodes.get(options.node);
this.node = node || this.manager.leastLoadNodes.first();
if (node) this.node = node
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
if (node) this.node = node
if (node) {this.node = node}

@@ -58,6 +58,11 @@ function check(options: NodeOptions) {
typeof options.retryDelay !== "number"
)
throw new TypeError('Node option "retryDelay" must be a number.');

if (typeof options.region !== "undefined" &&
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
if (typeof options.region !== "undefined" &&
if (typeof options.region !== "undefined" &&

return null;
else if (nodes.size === 1)
return nodes.first();
else if (nodes.size > 1)
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
else if (nodes.size > 1)
else if (nodes.size > 1)

return node.connected && node.options.region.toLowerCase() == region.toLowerCase()
});

if (!nodes)
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
if (!nodes)
if (!nodes)

@@ -119,7 +124,9 @@ export class Player {
if (options.textChannel) this.textChannel = options.textChannel;

const node = this.manager.nodes.get(options.node);
this.node = node || this.manager.leastLoadNodes.first();
if (node) this.node = node
else if (options.region) this.node = this.manager.nearestNode(options.region);
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
else if (options.region) this.node = this.manager.nearestNode(options.region);
else if (options.region) {this.node = this.manager.nearestNode(options.region);}

return node.connected
else if (Array.isArray(node.options.region))
return node.connected && node.options.region.includes(region)
else
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'else'.

Suggested change
else
else


if (!nodes)
return null;
else if (nodes.size === 1)
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
else if (nodes.size === 1)
else if (nodes.size === 1)

this.node = node || this.manager.leastLoadNodes.first();
if (node) this.node = node
else if (options.region) this.node = this.manager.nearestNode(options.region);
if (!this.node) this.node = this.manager.leastLoadNodes.first()
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
if (!this.node) this.node = this.manager.leastLoadNodes.first()
if (!this.node) {this.node = this.manager.leastLoadNodes.first()}

.filter((node) => {
if (!node.options.region)
return node.connected
else if (Array.isArray(node.options.region))
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
else if (Array.isArray(node.options.region))
else if (Array.isArray(node.options.region))

@@ -10,6 +10,9 @@ function check(options: PlayerOptions) {
throw new TypeError(
'Player option "guild" must be present and be a non-empty string.'
);

if (options.region && !/^\d+$/.test(options.region))
throw new TypeError('Player option "region" must be a non-empty string.');
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Strings must use doublequote.

Suggested change
throw new TypeError('Player option "region" must be a non-empty string.');
throw new TypeError("Player option \"region\" must be a non-empty string.");

if (typeof options.region !== "undefined" &&
((typeof options.region !== "string" ||
!/.+/.test(options.region)) && (!Array.isArray(options.region) || options.region.length === 0 || !/.+/.test(options.region[0]))))
throw new TypeError('Node option "region" must be a non-empty string or array.');
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Strings must use doublequote.

Suggested change
throw new TypeError('Node option "region" must be a non-empty string or array.');
throw new TypeError("Node option \"region\" must be a non-empty string or array.");

public nearestNode(region: string): Node {
const nodes = this.nodes
.filter((node) => {
if (!node.options.region)
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
if (!node.options.region)
if (!node.options.region)

@@ -10,6 +10,9 @@ function check(options: PlayerOptions) {
throw new TypeError(
'Player option "guild" must be present and be a non-empty string.'
);

if (options.region && !/^\d+$/.test(options.region))
Copy link
Member

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Expected { after 'if' condition.

Suggested change
if (options.region && !/^\d+$/.test(options.region))
if (options.region && !/^\d+$/.test(options.region))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants