search

jellz / warzone-stats

View Warzone info through Discord
4 stars 2 forks source link

Update index.js #4

Closed MrScopes closed 5 years ago

MrScopes commented 5 years ago

Now works with the most recent version of DJS, the API just needs to be public again.

Trophonix commented 5 years ago

Why did you change the MessageEmbed and Client syntax to using "Discord.RichEmbed()" everytime and change indents to tabs? Your PR doesn't just fix the issue, it changes the style of code... which makes for a bad PR. Unless you were talking to @danielgulic about this in private and this was a change you agreed on.

jellz commented 5 years ago

@Trophonix, I have not spoken with the PR author @MrScopes at all regarding these changes.

MrScopes commented 5 years ago

It didn't work on my end as I was getting an error @Trophonix. I'm on stable. I fixed it to the way that worked best for me. The error was on MessageCreate in DJS.

jellz commented 5 years ago

Hi, I have some issues with this pull request:

  1. The bot uses the master branch of Discord.js, as per the package.json. These changes would break the bot, as RichEmbed does not exist in the master branch.
  2. Why do you use const Discord = require('discord.js'); rather than const { Client, MessageEmbed } = require('discord.js'); ? What are the benefits? As far as I can see, there are none.
  3. You changed the ENTIRE FILE's indentation style to use tabs instead of spaces.

@Trophonix is right, you did just change the style of code, and didn't add or fix anything.

And as for your comment:

It didn't work on my end as I was getting an error @Trophonix. I'm on stable. I fixed it to the way that worked best for me. The error was on MessageCreate in DJS.

I'm confused, how could you be on stable when the package.json of this app specifies npm install/yarn to install the master branch of Discord.js?