-
Notifications
You must be signed in to change notification settings - Fork 8
Feature/embeds #8
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
base: rewrite
Are you sure you want to change the base?
Conversation
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.
Thanks for writing up the embed commands Veeru! I would change the structure of this cog a bit:
- Turn all the async functions inside
embed
into methods of theEmbeds
class - Move
check
,is_skip
andis_done
outsideembed
as well so they're accessible to the above
cogs/embeds.py
Outdated
@commands.command() | ||
async def embedraw(self, ctx: commands.Context): | ||
async def embedraw(self, ctx: commands.Context, *, embed_data): |
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.
embedraw
should take in a channel ID (where to send the embed) as its first argument.
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.
Do we always supply a channel_id in embedraw
? Even if we want to send the embed in the same channel?
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.
Disregard. Found an example and it seems that we do indeed send the channel_id: https://discord.com/channels/230296179991248896/349491266838462465/1049269313401524275
cogs/embeds.py
Outdated
""" | ||
Create a Discord embed via raw JSON input. | ||
""" | ||
raise NotImplementedError('Command requires implementation and permission set-up.') | ||
try: | ||
embed_body = json.loads(embed_data) |
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 implementation works, but it is not how embedraw
works within IB.ai - the JSON used there takes the fields
values as a list of 2-lists (a title string and a value string). However, I'm not sure if we want to preserve backwards compatibility or change this - maybe wait for feedback from Ray or Nathaneal.
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.
I think maintaining backwards compatibility would be good. Since I recall a lot of the embed_raw commands are pre-written and it'd be annoying to have to edit all of those.
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.
Can you provide me a sample snippet of the JSON that we are currently using for testing?
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.
Also, do we handle inline fields in the old method, and if not, should we be adding that functionality? It is a matter of parsing the JSON and checking whether the array has 2 elements or 3.
No description provided.