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

fix(Guild): type error with permissionOverwrites #10527

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Waheedsys
Copy link

close #10450

Please describe the changes this PR makes and why it should be merged:

This PR fixes an issue with permissionOverwrites in the Guild module, where a type error was thrown when setting permissions. The issue was caused by an incorrect handling of user and role IDs in certain scenarios.

The fix ensures that both user and role IDs are correctly resolved, preventing the type error and improving the stability of permission-related operations.

Closes issue: #10450

Status and versioning classification:

Code changes have been tested against the Discord API, or there are no code changes:
Yes, this PR involves code changes that have been tested. It improves error handling when setting permission overwrites, specifically handling cases where the id was not a User or Role.

I know how to update typings and have done so, or typings don't need updating:
Typings may need to be updated if the type field is now required when the id is a snowflake. Please review the typings and update accordingly.

Copy link

vercel bot commented Sep 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Oct 5, 2024 3:03pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Oct 5, 2024 3:03pm

@monbrey
Copy link
Member

monbrey commented Sep 30, 2024

Unless I'm the one misunderstanding the original issue, this PR doesn't seem to achieve anything - it's objectively worse.

  • The error message has not improved, it still implies that the id was not valid.
  • It makes unnecessary changes to the surrounding code to resolve the id.

Was this just a bad AI refactor of the existing code?

@Waheedsys
Copy link
Author

Thank you for the feedback, and I sincerely apologize if my changes missed the mark. This is my first contribution to this repository, and I'm still getting familiar with the codebase and conventions here.

I realize now that I might have misunderstood the original issue or approach. To ensure I address the problem correctly, I would appreciate a bit more time to review the original intent and work on refining my solution. I'll make sure the error handling is improved and unnecessary changes are reverted.

Thanks for your understanding, and I’ll provide an updated PR soon!

Copy link

@benjGam benjGam left a comment

Choose a reason for hiding this comment

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

Hi ! I guess @monbrey was mad at the fact that you do not improve in any way the report error, the bug is pretty much deep than just modifying the message.

If i get it, the original issue was talking about the fact that the provided overwrite wasn't "recognized" and so could not be treated correctly.

So the way to fix it, was by auto determine 'type' property in PermissionsOverwrites (or also use another way to resolve overwrite type, but it should work as it)

You didn't dig through code to understand and fix the bug, but you put a piece of tape on a bleeding wound ^^

But it's okay, it's your first contribution, you have the time to learn, anyway, thanks for that, next time, be more curious and try to understand the whole bug :)

@Waheedsys
Copy link
Author

@benjGam, Thanks for your feedback! I appreciate your insights, and I understand that my initial contribution didn’t fully address the root of the issue. I realize now that it’s important to dive deeper into the code and thoroughly understand the underlying problem rather than just applying a temporary fix.

I’ll make sure to take a more inquisitive approach in the future and work towards providing more comprehensive solutions. Thank you for your guidance, and I’m eager to improve with each contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

Type error with permissionOverwrites
3 participants