-
Notifications
You must be signed in to change notification settings - Fork 8
feat: insight cog #423
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: insight cog #423
Conversation
Codecov Report
@@ Coverage Diff @@
## master #423 +/- ##
==========================================
+ Coverage 87.35% 87.49% +0.14%
==========================================
Files 125 130 +5
Lines 9110 9214 +104
==========================================
+ Hits 7958 8062 +104
Misses 1152 1152
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, main thing is to use core.py purely to try to conform to the new approach we are taking to file structure
had missed out a space in a message to be sent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorreee little more to change, last thing I promise
koala/cogs/insights/core.py
Outdated
partial_message += f", {guild}" | ||
messages.append(partial_message) | ||
else: | ||
return [f"No servers found containing the string \"{filter_string}\"."] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just return empty list, and have cog.py deal with sending this message if result = []
koala/cogs/insights/cog.py
Outdated
""" | ||
|
||
for message in get_servers(self.bot, filter_string): | ||
await ctx.send(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use ctx.send this many times for one request as we could hit the ratelimit. We should instead group up to 2k characters into one message using \n then if more than 2k characters move to new message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Summary
close #95
Checklist
CHANGELOG.md
under the[Unreleased]
heading?documentation.json
?