Skip to content

Add get or fetch util #1837

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Oct 3, 2021
Merged

Add get or fetch util #1837

merged 6 commits into from
Oct 3, 2021

Conversation

ChrisLovering
Copy link
Member

This is now needed, as we have noticed that it's not guaranteed that the member cache will always be fully populated. Leading to members not having their help cooldown roles removed.

wookie184
wookie184 requested changes Sep 20, 2021
View reviewed changes
@ChrisLovering ChrisLovering force-pushed the add-get-or-fetch-util branch 3 times, most recently from d1bb951 to 4d01e9b Compare September 20, 2021 21:23
wookie184
wookie184 previously approved these changes Sep 21, 2021
View reviewed changes
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.

LGTM

TizzySaurus
TizzySaurus reviewed Sep 21, 2021
View reviewed changes
Copy link
Contributor

@TizzySaurus TizzySaurus left a comment

Choose a reason for hiding this comment

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

There's a possible missed migration here (L143 of bot/exts/utils/reminders.py).

Also not sure but should the converter instead catch discord.ext.commands.MemberNotFound (the actual error rather than a parent class)?

@ChrisLovering
Copy link
Member Author

ChrisLovering commented Sep 21, 2021 •
edited
Loading

There's a possible missed migration here (L143 of bot/exts/utils/reminders.py).

I purposefully skipped that since it's an generator, so I'd need to convert it to an async generator, so that cna be done in a different PR.

Also not sure but should the converter instead catch discord.ext.commands.MemberNotFound (the actual error rather than a parent class)?

This isn't a convertor, this is calling discord.Guild.fetch_member(), which doesn't raise that error afaik. IIRC discord.ext.commands.MemberNotFound subclasses BadArgument rather than discord.errors.NotFound

@TizzySaurus
Copy link
Contributor

I purposefully skipped that since it's an generator, so I'd need to convert it to an async generator, so that cna be done in a different PR.

This isn't a convertor, this is calling Guild.fetch_member, which doesn't raise that error afaik.

Looks good then. There's also the fact that the equivalent util for channels is called try_get_channel rather than get_or_fetch. Do we want to change one of these for consistency's sake or just leave it?

@TizzySaurus TizzySaurus mentioned this pull request Sep 21, 2021
Merged
@ChrisLovering ChrisLovering force-pushed the add-get-or-fetch-util branch 3 times, most recently from abccccd to 34484a8 Compare September 21, 2021 19:47
ToxicKidz
ToxicKidz approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@ToxicKidz ToxicKidz left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

kwzrd
kwzrd reviewed Sep 21, 2021
View reviewed changes
Copy link
Contributor

@kwzrd kwzrd left a comment

Choose a reason for hiding this comment

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

Have you considered making the helper a bot instance method, having the guild argument optional and defaulting to pydis? In some cases, caller code could then have:

bot.get_or_fetch_member(1234)

Instead of:

import guild id
import get_or_fetch_member

guild = self.bot.get_guild(guild id)
get_or_fetch_member(guild, member id)

I haven't thought about it much, maybe this is a bad idea.

@TizzySaurus
Copy link
Contributor

I haven't tested anything so won't formally approve, but after a few things discussed on Discord this all looks good now 👍

@ChrisLovering ChrisLovering dismissed wookie184’s stale review September 21, 2021 20:35

It's changed quite a bit since this approval

@Xithrius Xithrius requested a review from wookie184 September 21, 2021 21:58
@Xithrius Xithrius added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: help channels Related to the help channel system p: 1 - high High Priority s: needs review Author is waiting for someone to review and approve labels Sep 21, 2021
@Xithrius Xithrius added the t: enhancement Changes or improvements to existing features label Sep 21, 2021
@ChrisLovering
Add get_or_fetch_member util
b9f41c5 
This is now needed, as we're a large server it's not guaranteed that the member cache will always be fully populated.
@ChrisLovering
Migrate all Guild.get_member calls to get_or_fetch_member
15a8e7e 
This protects us against the guild cache not being fully populated with members.
@ChrisLovering
Mock fetch_member in tests that user get_or_fetch now
7a9828b
@ChrisLovering
Rename channel helper to be consistent with other helpers
6cf9f54
@ChrisLovering
Add new poetry tasks for pytest
e52db75 
Updated the test task to now run with --ff which runs failed tests from the last run first

Added retest, which runs pytest with --lf this only runs the failed tests from the last test run
wookie184
wookie184 approved these changes Oct 3, 2021
View reviewed changes
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.

LGTM, tested and seems to work smoothly

@wookie184 wookie184 merged commit 5f657a0 into main Oct 3, 2021
@wookie184 wookie184 deleted the add-get-or-fetch-util branch October 3, 2021 13:38
@Xithrius Xithrius removed the s: needs review Author is waiting for someone to review and approve label Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwzrd kwzrd kwzrd left review comments

@TizzySaurus TizzySaurus TizzySaurus left review comments

@wookie184 wookie184 wookie184 approved these changes

@ToxicKidz ToxicKidz ToxicKidz approved these changes

@Akarys42 Akarys42 Awaiting requested review from Akarys42

@Den4200 Den4200 Awaiting requested review from Den4200

@jb3 jb3 Awaiting requested review from jb3

@ks129 ks129 Awaiting requested review from ks129

@MarkKoz MarkKoz Awaiting requested review from MarkKoz MarkKoz is a code owner

@mbaruh mbaruh Awaiting requested review from mbaruh mbaruh is a code owner

Assignees
No one assigned
Labels
a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: help channels Related to the help channel system p: 1 - high High Priority t: enhancement Changes or improvements to existing features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ChrisLovering @TizzySaurus @wookie184 @kwzrd @ToxicKidz @Xithrius