-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add warning to [p]set api if <>s are included in secret #6265
base: V3/develop
Are you sure you want to change the base?
Conversation
@Drapersniper I added the changes requested in your comments as another commit on the PR, thanks |
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.
Just a couple of suggested changes to improve readability.
"You may have failed to properly format your {api_service_name}. If you were told to enter a key" | ||
" with an example such as [p]set api {service} api_key <your_api_key_here>, and your API key" | ||
" was HREDFGWE, make sure to run [p]set api {service} api_key HREDFGWE, and not " | ||
" [p]set api {service} api_key <HREDFGWE>" |
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.
Wrapping the command examples with inline marks may be appropriate to improve readability.
"You may have failed to properly format your {api_service_name}. If you were told to enter a key" | |
" with an example such as [p]set api {service} api_key <your_api_key_here>, and your API key" | |
" was HREDFGWE, make sure to run [p]set api {service} api_key HREDFGWE, and not " | |
" [p]set api {service} api_key <HREDFGWE>" | |
"You may have failed to properly format your {api_service_name}. If you were told to enter a key" | |
" with an example such as `[p]set api {service} api_key <your_api_key_here>`, and your API key" | |
" was `HREDFGWE`, make sure to run `[p]set api {service} api_key HREDFGWE` and not " | |
" `[p]set api {service} api_key <HREDFGWE>`." |
await ctx.send(_("`{service}` API tokens have been set.").format(service=service)) | ||
await ctx.send( | ||
_("`{service}` API tokens have been set.").format(service=service), | ||
embed=embed if len(embed.fields) > 0 else None, |
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.
It isn't necessary to give this warning inside of an embed, when there is already text content (it would make things more complicated, such as having to check if an embed is requested in the current channel). Instead, you could add line breaks under the original message and put the warning text there. Something like this would suffice:
angle_bracket_warning = None # define this before iterating through tokens
for api_service_name, token in tokens.items():
...
message = _("`{service}` API tokens have been set.").format(service=service)
if angle_bracket_warning:
message += "\n\n" + _("**Warning:** ") + _(angle_bracket_warning)
await ctx.send(message)
Remember to remove the embed
definition on line 3636.
Description of the changes
Closes #6253
For every given secret, check if its wrapped in <>, if it is, add a message to an embed with a warning that that secret may not be set properly. I tested this on a local instance of the bot and it seems to be working as expected. The warning is also logged
Have the changes in this PR been tested?
Yes