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

Change Plain Text to Embed #20

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

kphoenix137
Copy link
Contributor

@kphoenix137 kphoenix137 commented Oct 2, 2023

diablo_retail
diablo_spawned
hellfire_retail
hellfire_spawned

Comment on lines +121 to +124
diablo_retail_url = 'https://user-images.githubusercontent.com/68359262/272125642-d3363701-9b6e-4eff-b43e-11b9a0b58813.png'
diablo_shareware_url = 'https://user-images.githubusercontent.com/68359262/272125649-076eb58c-f587-4b16-822f-7d9b56559c69.png'
hellfire_retail_url = 'https://user-images.githubusercontent.com/68359262/272125658-3fc00693-e6dc-4273-81e8-803dbf9f5d93.png'
hellfire_shareware_url = 'https://user-images.githubusercontent.com/68359262/272125665-17805298-1400-49aa-a6a3-ac80ba5767ee.png'
Copy link
Member

Choose a reason for hiding this comment

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

I feel it would be better to upload the images to the repo and reference those URLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does AJenbo have to do that?

Copy link
Member

Choose a reason for hiding this comment

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

You can add the files to your PR and preemptively determine what the URLs would be once merged. Or break it into two PRs so the graphics can be added first.

'LTHR': hellfire_retail_url, # Lord of Terror Hellfire Retail (kphoenix)
'LTHS': hellfire_shareware_url, # Lord of Terror Hellfire Shareware (kphoenix)
}
embed["thumbnail"]["url"] = game_type_icons.get(game['type'], "")
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the empty string would cause an exception. Since the thumbnail is optional, we should remove it completely if the URL would be invalid.

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 think we should use a placeholder graphic. Maybe the retail diablo icon, or something else as a placeholder for unknown game id

Copy link
Member

Choose a reason for hiding this comment

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

If so, I would avoid using an icon we're already using. However, I would caution against making unknown game types recognizable. Players will use the icon to differentiate between game types, but unknown game types would all share the same icon. Maybe a question mark would be appropriate, but I think no icon is perfectly reasonable.

But honestly, for all we know, the unknown game type could be an attacker trying to confuse or crash the bot. The main thing is to avoid exceptions that would compromise the main loop.

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'll come up with something good

text += ' Hell'

embed = {
"type": "rich",
Copy link
Member

Choose a reason for hiding this comment

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

Let's change all double-quotes to single-quotes. It's more consistent with the rest of the file.

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