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: make Message#author lazy #204

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

Conversation

biqqles
Copy link
Contributor

@biqqles biqqles commented Mar 6, 2023

Summary

Fixes #117. Calling Message#author will still result in 'Unknown Member' being printed if the member does not exist (don't think there's a way to hide this? It is kinda annoying), but it won't hang instantiators of Message such as Channel#history.

This is a bit rough right now but functional, will make a cleanup pass when I have more time.


Fixed

  • Reimplemented Message#author as a method instead of an attribute

member = @channel.server.member(data['author']['id'].to_i)

if member
member.update_data(data['member']) if data['member']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From a very quick check, data['member'] doesn't seem to exist anymore so I ignored the code relating to it.

Copy link
Member

Choose a reason for hiding this comment

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

data['member'] is present when messages are received via a gateway event

lib/discordrb/data/message.rb Outdated Show resolved Hide resolved
member = @channel.server.member(data['author']['id'].to_i)

if member
member.update_data(data['member']) if data['member']
Copy link
Member

Choose a reason for hiding this comment

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

data['member'] is present when messages are received via a gateway event

@@ -390,5 +357,36 @@ def buttons

results.flatten.compact
end

# The user who sent this message message. Will be a {Member} most of the time, a {User} in the case
# that the user has left the server, or a {Recipient} if the message was sent in a private channel.
Copy link
Member

Choose a reason for hiding this comment

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

Please add a type doc here

author = @channel.server.member(author_id) || @bot.user(author_id)
case author
when Member
author.update_data(@author_data)
Copy link
Member

Choose a reason for hiding this comment

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

Because we're handling this lazily now we should not update at this point in the lifecycle. There is no guarantee that our data is more up to date than the cache

Copy link
Member

Choose a reason for hiding this comment

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

We may need to ensure that we're updating/creating the member during the MessageCreate event processing though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this what ensure_user is for?

@swarley
Copy link
Member

swarley commented Mar 6, 2023

Thanks for the PR! Just some minor things to work through before we get it merged

# Conflicts:
#	lib/discordrb/data/message.rb
Copy link
Member

@swarley swarley left a comment

Choose a reason for hiding this comment

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

Last two requests on this one in hopes of avoiding more network requests if possible

lib/discordrb/data/message.rb Show resolved Hide resolved
# directly because the bot may also send messages to the channel
Recipient.new(@bot.user(author_id), @channel, @bot)
else
author = @channel.server.member(author_id) || @bot.user(author_id)
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we should use ensure_member/ensure_user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

member is cached by default but made the other changes

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.

Error: Unknown Member in Channel#history
2 participants