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

Make Guild.member_count Optional #7605

Merged
merged 4 commits into from
Mar 10, 2022

Conversation

AbstractUmbra
Copy link
Contributor

Summary

According to multiple reports, this key is not always present in a guild payload. Currently the library only sets this attribute if it is not None resulting in a rare error during startup or when retrieving guilds.
Potentially a fix for #6652?

I have made this attribute and property Optional as a result, to avoid an AttributeError.

Sadly I cannot test this, due to potentially requiring an "unavailable" guild.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@Rapptz
Copy link
Owner

Rapptz commented Mar 9, 2022

I don't know how I feel about the type of member_count changing to Optional[int]. Python ergonomics with optional types is subpar, especially when the data is always there. Not sure what the solution is but it is unfortunate.

@dolfies
Copy link

dolfies commented Mar 9, 2022

Unless I'm misremembering, the only time member_count will not be present is when a guild is unavailable. In that case, can't we just default to 0? You could argue it actually is 0 since no-one can access it.

@EvieePy
Copy link
Contributor

EvieePy commented Mar 9, 2022

I don't think 0 makes a lot of sense here. The guild member count is still above 0, regardless of whether it is unavailable or not.
If you were going to return 0 you might as well just return None for some sanity. But I'm not sure... So goodlock with this.

@dolfies
Copy link

dolfies commented Mar 10, 2022

None will make typing UX not amazing.

@AbstractUmbra
Copy link
Contributor Author

I am aware that marking this attribute as Optional is a far from ideal solution, however I believe that it's the lesser of two evils, considering the other option (at the moment) is a rare AttributeError that's been affecting people.

I'd rather a little extra work to appease a type checker than it just dying, personally.

Copy link
Owner

@Rapptz Rapptz left a comment

Choose a reason for hiding this comment

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

Needs updating migrating.rst

discord/guild.py Outdated Show resolved Hide resolved
@dolfies
Copy link

dolfies commented Mar 10, 2022

I am aware that marking this attribute as Optional is a far from ideal solution, however I believe that it's the lesser of two evils, considering the other option (at the moment) is a rare AttributeError that's been affecting people.

I'd rather a little extra work to appease a type checker than it just dying, personally.

That's why 0 is a good middle ground. In the rare event it isn't provided, it being 0 shouldn't be a problem. Even name defaults to an empty string.

@Rapptz Rapptz merged commit 03687fb into Rapptz:master Mar 10, 2022
@AbstractUmbra AbstractUmbra deleted the fix/guild-member-count branch March 17, 2022 13:22
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.

4 participants