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

Bunnies, Trivia, Trump and Yo Mama Commands. #23

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from

Conversation

ImCrispyy
Copy link
Contributor

Thank you KhaaZ. Hopefully there are less and less problems in these commands, if there are any... hammer me down 😸

async execute({ message }) {
try {
const utils = this.utils;
const errorText = `Error: ${this.config.emojis.sadcat || '🐰'} No bunnies found.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

uses sadcat emoji here, shouldn't

},
});
} catch(err) {
return this.error(message.channel, errorText);
Copy link
Contributor

Choose a reason for hiding this comment

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

errorText is not defined in the proper scope to be referenced here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do I remove it then? Or will I have to define it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just fix it, the var errorText is not defined in a scope thats accessible here

async execute({ message }) {
try {
const utils = this.utils;
const errorText = `Error: ${this.config.emojis.rabbit || '🐰'} No bunnies found.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

just move this up 3 lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by move? I can't see an error.

@ImCrispyy
Copy link
Contributor Author

Done?

@briantanner
Copy link
Contributor

No, review changes were never finished/fixed

@ImCrispyy
Copy link
Contributor Author

YES I DID IT (thanks KhaaZ 🍳 )


async execute({ message }) {
const errorText = `Error: ${this.config.emojis.rabbit || '🐰'} No bunnies found.`;
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent spacing between errorText and try {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

image: {
url: `https://random.birb.pw/img/${res.body.media.poster}`,
},
url: `https://random.birb.pw/img/${res.body.media.poster}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work. The return values for the bunnies api is:

{"id":"141","media":{"poster":"https://bunnies.media/poster/141.png"},"source":"unknown","thisServed":708,"totalServed":830204}

Which means that this willl resolve to something like this:
https://random.birb.pw/img/$https://bunnies.media/poster/141.png

And thus, will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So remove the .poster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh crap i should of fixed that a while ago

async execute({ message }) {
try {
let res = await superagent.get('http://api.yomomma.info/');
return this.sendMessage(message.channel, JSON.parse(res.text).joke);
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent spacing (missing tab before return)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought i fixed it 🤔

{ search: 'Looking for a bunnies...', found: 'Found one!' },
];

const response = responses[utils.getRandomInt(0, responses.length - 1)];
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like it's needed, since we only have a single response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

const response = responses[utils.getRandomInt(0, responses.length - 1)];
const msg = await this.sendMessage(message.channel, response.search);

let res = await superagent.get('https://api.bunnies.io/v2/loop/random/?media=poster');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to group this.sendMessage() and superagent.get() in a Promise.all([]) so we're not waiting for the message to send before querying the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How will that be formatted? I've yet to write a Promise.all statement,

Copy link
Contributor

@germanoeich germanoeich Mar 10, 2018

Choose a reason for hiding this comment

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

Here's an example:

let [iss, issPeople] = await Promise.all([
superagent.get('http://api.open-notify.org/iss-now.json'),
superagent.get('http://api.open-notify.org/astros.json'),
]);
iss = iss.body;
issPeople = issPeople.body;

@ImCrispyy
Copy link
Contributor Author

Done.

@ImCrispyy
Copy link
Contributor Author

Anything else friends?

@briantanner
Copy link
Contributor

it says there's requested changes ;)

@ImCrispyy ImCrispyy changed the title 4 more commands! Thank KhaaZ for all the help, without him the commands wouldn't be functional Bunnies, Trivia, Trump and Yo Mama Commands. Mar 20, 2018
@ImCrispyy
Copy link
Contributor Author

Last of the bug fixes and It looks like I have finished. Someone confirm that everything has been fixed?

@ImCrispyy
Copy link
Contributor Author

Are these commands going to get check? They don't have to become commands; just let me know. :((

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.

4 participants