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

Bugfix - Discordrb::Message#reacted_with #288

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

y-bonfire
Copy link

Summary

I noticed this bug recently.
#60

Upon investigation, I realized that the issue stems from a misunderstanding of how proc works. Returning inside a proc causes it to return a value immediately, which prevents the loop from continuing. This appears to be a basic Ruby mistake.

After applying the fix, I ran the code locally and confirmed that it successfully retrieves users with more than 100 reactions.

Here is a simple sample to illustrate the issue.

def proc_return_test

  proc_test = proc do |a|
    return a
  end

  val = proc_test.call(1)
  puts val
  0
end

def proc_next_test

  proc_test = proc do |a|
    next a
  end

  val = proc_test.call(1)
  puts val
  0
end


# 1
puts proc_return_test

# 1,0
puts proc_next_test

@y-bonfire
Copy link
Author

@swarley @Daniel-Worrall review please

@Daniel-Worrall
Copy link
Member

@y-bonfire Was the code tested before the change as well? The code changed in the PR originates after the issue was posted, and the issue may not even still be applicable

@y-bonfire
Copy link
Author

@y-bonfire Was the code tested before the change as well? The code changed in the PR originates after the issue was posted, and the issue may not even still be applicable

@Daniel-Worrall
Yes, I confirmed that for cases like 600 reactions, the code previously only retrieved 100. After the change, it now retrieves the expected values as intended.

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.

3 participants