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: always cast PartialMessage(able) IDs to int #2406

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NeloBlivion
Copy link
Member

@NeloBlivion NeloBlivion commented Mar 21, 2024

Summary

This was initially a PR to fix broken views sent in create_thread, but it turned out to be an underlying issue... once again with string IDs still being present in some areas of the library. ForumChannel.create_thread used the raw string ID for the ViewStore key, resulting in it always failing to resolve. Instead of casting int here, I opted to force PartialMessage and PartialMessageable to always convert to int.

Information

  • 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, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • 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.
  • I have updated the changelog to include these changes.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -3189,7 +3189,7 @@ def __init__(
):
self._state: ConnectionState = state
self._channel: Object = Object(id=id)
self.id: int = id
self.id: int = int(id)
Copy link
Member

@Dorukyum Dorukyum Mar 22, 2024

Choose a reason for hiding this comment

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

Why not change the part where the ID is passed in with the wrong type instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose, but shouldn't we guarantee all IDs are int for parity with full channel objects?

Copy link
Member

Choose a reason for hiding this comment

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

Or we could change both

Copy link
Member

Choose a reason for hiding this comment

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

The library usually converts before passing in, so I think we should continue that

Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
Signed-off-by: UK <41271523+NeloBlivion@users.noreply.github.com>
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.

None yet

4 participants