Skip to content

Catch error in case of non successfull API call for new ot-names#448

Merged
sco1 merged 8 commits into
python-discord:masterfrom
Akarys42:ot-fix
Sep 24, 2019
Merged

Catch error in case of non successfull API call for new ot-names#448
sco1 merged 8 commits into
python-discord:masterfrom
Akarys42:ot-fix

Conversation

@Akarys42
Copy link
Copy Markdown
Contributor

This allows to keep the task running even if the call fail.

This PR closes #442

This allows to keep the task running even if the call fail.
Comment thread bot/cogs/off_topic_names.py Outdated
@MarkKoz MarkKoz added area: cogs t: bug Something isn't working labels Sep 24, 2019
@Akarys42
Copy link
Copy Markdown
Contributor Author

I added a continue instead of a return to keep the code running

Copy link
Copy Markdown
Contributor

@SebastiaanZ SebastiaanZ left a comment

Choose a reason for hiding this comment

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

This looks promising, although it won't work in the current form. See my review comment.

From a more meta-perspective, one consequence this approach will have is that if we fail to update the off-topic names due to some temporary outage, they won't be updated until we hit the next UTC midnight. This is not that bad, but we could also think of rescheduling the name change, say, half an hour later if it fails instead of waiting a whole day to try again.

That said, the current changes will already prevent the task from failing completely (minus the thing in the review comment).

Comment thread bot/cogs/off_topic_names.py Outdated
`bot.api.ResponseCodeError` is now imported
If an exception occurred
@Akarys42
Copy link
Copy Markdown
Contributor Author

Import error fixed and waiting time reduced to half an hour if an exception occurred.

Copy link
Copy Markdown
Contributor

@SebastiaanZ SebastiaanZ left a comment

Choose a reason for hiding this comment

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

Hey @Akarys42,

Your general fix is very solid, thanks.

The retry approach works well as well, but we had a conversation about the OffTopicNames cog in the core-dev team and decided that we currently don't want to use retry logic for the off-topic names. I know I was the one who suggested it in the first place, but could you remove it from your PR?

The main reason is that we're thinking of an alternative approach for the entire cog, now that we've got access to features like the new discord.py Tasks. We also want to look into adding more admin control to the off-topic name cog and another way of handling failing connections to the API.

Overall, the core fix is very solid, though, so we do really want to merge that.

After a short discussion in the core-dev team, we decided to not use
retry logic for a failed API call for new off-topic-names. We may
introduce something similar in the future, but we're not sure on the
direction we want to take yet.
@sco1 sco1 merged commit d6ab11e into python-discord:master Sep 24, 2019
@Akarys42 Akarys42 deleted the ot-fix branch September 24, 2019 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The off-topic channel name updating task fails on non-success API response

4 participants