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

feat: Added "!insult" command #124

Merged
merged 4 commits into from
Jul 7, 2019
Merged

feat: Added "!insult" command #124

merged 4 commits into from
Jul 7, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jun 4, 2019

This command will pick a random insult, and send on the channel. You can
also select the person to insult too.
Example:

!insult
!insult @someone
!insult @foo @bar

This command will pick a random insult, and send on the channel. You can
also select the person to insult too.
Example:
```
!insult
!insult @Someone
!insult @foo @bar
```
@ghost ghost requested a review from bramz June 4, 2019 07:51
Copy link
Contributor

@bramz bramz left a comment

Choose a reason for hiding this comment

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

Looks clean, my only request is a possible name change for this. Some suggestions/ideas abuse, taunt, offend? I suppose the term insult could seem a bit harsh and maybe we should take a lighter approach here, I'm leaning more towards, taunt.

Also, if you could amend your commit message to remove feat:. This is simply my OCD at play here. I think it looks a bit cleaner, basically, all my commits follow this style. I am aware agro's do not but I plan on asking him to follow in future as well.

@@ -0,0 +1,61 @@
Just what do you think you're doing Dave?
Copy link

Choose a reason for hiding this comment

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

These insults are terrible.

Copy link
Author

Choose a reason for hiding this comment

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

We can always edit it though.

@ghost
Copy link
Author

ghost commented Jun 4, 2019

Due to lack of better insults, I am cancelling the PR. Will submit again after I find a good list of insults.

@ghost ghost closed this Jun 4, 2019
@bramz
Copy link
Contributor

bramz commented Jun 4, 2019

So as we discussed on discord, feature functionality could be something similar to the following.

!insult output random insult at the user who triggered insult
!insult @user output random insult at specified user
!insult add <insult here> add insult to database
!insult del <id> delete insult from database

Add a table to migrations, and use postgres or you can simply append to the flat file, your choice.

@bramz bramz reopened this Jun 4, 2019
@bramz
Copy link
Contributor

bramz commented Jun 4, 2019

Consider this schema, extend upon this or simply add it to migrations.

CREATE TABLE insults (
  id serial primary key,
  creator text not null,
  insult text not null
);

Instead of a file to hold insults, DB will be used from now on.
Copy link
Contributor

@bramz bramz left a comment

Choose a reason for hiding this comment

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

As you mentioned, the addition of an insulting message is not capped, thus allowing the full 2k message limit of discord to be used. A max length of 500 for insulting messages should suffice.

@@ -9,6 +9,7 @@
!insult add a new insult
!insult del <integer>
```"""
limit = 500 # Not more than 500 characters insult
Copy link
Contributor

Choose a reason for hiding this comment

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

You should declare this as a constant.

Copy link
Author

Choose a reason for hiding this comment

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

Okay

@bramz bramz merged commit c095ec2 into master Jul 7, 2019
@bramz bramz deleted the insults branch July 7, 2019 15:12
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