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

Use Guild member counts rather than user cache size #182

Merged
merged 10 commits into from
Mar 25, 2021
Merged

Use Guild member counts rather than user cache size #182

merged 10 commits into from
Mar 25, 2021

Conversation

Lioness100
Copy link
Contributor

This is mostly a fix for discord.js v12+

Currently, DiscordJS#userCount relies on the user cache, which will result in inaccurate numbers. (Much) more often than not, the user cache will not include every single user. Instead, it would be better to use the guild cache (since by default they're all guaranteed to be there) using Collection#reduce() and Guild#memberCount

@EndBug EndBug self-assigned this Mar 24, 2021
@EndBug EndBug added good first issue Good for newcomers type: feature New feature or feature request labels Mar 24, 2021
@EndBug
Copy link
Member

EndBug commented Mar 24, 2021

Hi, thanks for the PR!
At the moment I'm waiting to marge a PR that changes how the lib folder is handled, so we'll need to update this PR after I merge that.

Anyway, regarding your contribution: the change looks good to me. The tests for Poster#post are failing, the dummy client should be updated to fit the new way of getting the user count.

@Snazzah
Copy link
Member

Snazzah commented Mar 25, 2021

This change should be mirrored to the other client fillers if possible. I'm pretty sure the other client fillers rely on user cache rather than member count.

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #182 (7a43b33) into master (d55d45c) will decrease coverage by 0.56%.
The diff coverage is 50.00%.

❗ Current head 7a43b33 differs from pull request most recent head 014fca0. Consider uploading reports for the commit 014fca0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
- Coverage   96.98%   96.42%   -0.57%     
==========================================
  Files          45       45              
  Lines        1196     1204       +8     
  Branches      191      191              
==========================================
+ Hits         1160     1161       +1     
- Misses         32       39       +7     
  Partials        4        4              
Impacted Files Coverage Δ
src/Interface/ClientFiller.ts 100.00% <ø> (ø)
src/Interface/Clients/DiscordIO.ts 70.00% <25.00%> (-7.78%) ⬇️
src/Interface/Clients/Eris.ts 88.23% <50.00%> (-5.52%) ⬇️
src/Interface/Clients/Paracord.ts 86.66% <50.00%> (-13.34%) ⬇️
src/Interface/Clients/DiscordJS.ts 81.81% <60.00%> (-3.19%) ⬇️
src/Interface/Clients/Discordie.ts 87.50% <66.66%> (-5.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d55d45c...014fca0. Read the comment docs.

Copy link
Member

@EndBug EndBug left a comment

Choose a reason for hiding this comment

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

@Snazzah Maybe it's better if we merge this PR and change only discord.js for now.
Then we can always change the other ones too, but maybe it's better if you do it, I'm not really that familiar with the other libraries.

@Snazzah Snazzah changed the title Use Guild#memberCount instead of relying on User collection size Use Guild member counts rather than user cache size Mar 25, 2021
@Snazzah Snazzah requested a review from EndBug March 25, 2021 15:23
@Lioness100
Copy link
Contributor Author

This looks great!

Copy link
Member

@EndBug EndBug left a comment

Choose a reason for hiding this comment

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

Awesome!

src/Interface/Clients/DiscordIO.ts Show resolved Hide resolved
Snazzah and others added 2 commits March 25, 2021 13:13
@Snazzah Snazzah merged commit b9a78a5 into dbots-pkg:master Mar 25, 2021
@EndBug
Copy link
Member

EndBug commented Mar 25, 2021

@Lioness100 All set then! Thanks again for your contribution! The fix will be published in the next version 👍🏻

@EndBug EndBug added type: fix Updates to existing functionalities and removed type: feature New feature or feature request labels Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: fix Updates to existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants