Skip to content

Conversation

Tr-Do
Copy link

@Tr-Do Tr-Do commented Aug 13, 2025

Relevant Issues

https://github.com//issues/1691?notification_referrer_id=NT_kwDOBivkC7UxODE0MTkwMjE5MjoxMDM1Mzk3MjM#event-19124053085

Description

Changes made:

  • Extend the existing 'joke' command with 'dad' category using icanhazdadjoke API
  • Keep the existing joke categories (chuck, neutral, all)
    How it's implemented:
  • When dad joke is called, an asynchronous HTTP GET request is made to https://icanhazdadjoke.com with aiohttp
  • Implement user agent to comply with icanhazdadjoke API policy
  • The joke from JSON is sent back to the user with ctx.send()
  • Linted and tested in a new python file before adding the code to fun.py

Did you:

Copy link
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the slow review, this looks great, thank you!

I have two minor comments.

Comment on lines 165 to 170
async with aiohttp.ClientSession() as session, session.get(
"https://icanhazdadjoke.com",
headers={
"Accept":"application/json",
"User-Agent": "Sir-lancebot"
}) as res:
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Reusing the bot's internal http_session is more efficient than creating a new one.
  • Including a contact method (e.g. the URL to the project) in the user agent is good practice.
Suggested change
async with aiohttp.ClientSession() as session, session.get(
"https://icanhazdadjoke.com",
headers={
"Accept":"application/json",
"User-Agent": "Sir-lancebot"
}) as res:
async with self.bot.http_session.get(
"https://icanhazdadjoke.com",
headers={
"Accept": "application/json",
"User-Agent": "Sir-Lancebot (https://github.com/python-discord/sir-lancebot)",
}
) as res:

data = await res.json()
await ctx.send(data["joke"])
else:
await ctx.send("There is no dad joke now")
Copy link
Contributor

Choose a reason for hiding this comment

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

This path indicates something is wrong, it's probably a good idea to raise an error so it will be logged and we can look into it. res.raise_for_status() can be used to do this.

Copy link
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Thanks!

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