Skip to content

Conversation

krittick
Copy link
Contributor

@krittick krittick commented Mar 1, 2022

Summary

This adds a new private method Guild._fetch_role() which allows utils.get_or_fetch to work with roles properly. Previously it did not work because there was only a fetch_roles method, not fetch_role. As part of this change, utils.get_or_fetch was also updated to use private _fetch methods if an AttributeError is raised for any object that does not have a fetch_* method. This has an added benefit of allowing us to more easily add private _fetch methods on other objects if we need to in the future.

This also fixes #906 by having the "mentionable" option type check for first a user, then a role if a user is not found. This required the above described addition of Guild._fetch_fole() to work properly.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why
  • 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, typehinting, examples, ...)

@krittick krittick self-assigned this Mar 1, 2022
@krittick krittick added bug Something isn't working Merge with squash priority: high High Priority status: awaiting review Awaiting review from a maintainer labels Mar 1, 2022
@krittick krittick added this to the v2.0 milestone Mar 1, 2022
@krittick krittick enabled auto-merge (squash) March 1, 2022 20:09
Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
@krittick krittick requested a review from Dorukyum March 1, 2022 20:44
Dorukyum
Dorukyum previously approved these changes Mar 1, 2022
Copy link
Contributor

@zeffo zeffo left a comment

Choose a reason for hiding this comment

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

Perhaps the note should be changed to This method is an API call. For general usage, consider using :attr:`get_role` instead.

@BobDotCom
Copy link
Contributor

I feel like making this a private method "_fetch_role" would be more correct (And add handling for that inside get_or_fetch). This currently doesn't reflect the api well and shouldn't really be a method we promote, as it's rather "fetch_roles_and_filter_for_one_role"

Copy link
Contributor

@BobDotCom BobDotCom left a comment

Choose a reason for hiding this comment

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

See my last comment

@krittick
Copy link
Contributor Author

krittick commented Mar 4, 2022

I feel like making this a private method "_fetch_role" would be more correct (And add handling for that inside get_or_fetch). This currently doesn't reflect the api well and shouldn't really be a method we promote, as it's rather "fetch_roles_and_filter_for_one_role"

Good point. I'll update with those changes and test it out. Setting to draft for now.

@krittick krittick added status: in progress Work in Progess and removed status: awaiting review Awaiting review from a maintainer labels Mar 4, 2022
@krittick krittick disabled auto-merge March 4, 2022 15:32
@krittick krittick marked this pull request as draft March 4, 2022 15:33
@krittick krittick marked this pull request as ready for review March 10, 2022 22:10
@krittick krittick changed the title Add Guild.fetch_role() method to support proper handling of role mentionable in slash command options Add Guild._fetch_role() private method to support proper handling of role mentionable in slash command options Mar 10, 2022
@krittick
Copy link
Contributor Author

This is ready for review now.

@krittick krittick enabled auto-merge (squash) March 10, 2022 22:16
@krittick krittick added status: awaiting review Awaiting review from a maintainer and removed status: in progress Work in Progess labels Mar 10, 2022
Copy link
Member

@Icebluewolf Icebluewolf left a comment

Choose a reason for hiding this comment

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

I downloaded this branch and It fixed the problem for me!

@krittick krittick merged commit fbdf1bb into Pycord-Development:master Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: high High Priority status: awaiting review Awaiting review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slash command option input type "mentionable" issue
6 participants